Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prior Parameter Distribution and Transform Visuals + Adstock Weighting and Contribution Visuals (Draft) #477

Draft
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

eirikbaekkelund
Copy link

@eirikbaekkelund eirikbaekkelund commented Jan 7, 2024

Description

Additional functionalities serving to help the users specify their priors. This includes functions to:

  1. Explore the priors across channels
  2. See the effect of the adstock- and saturation transforms across channels
  3. Compare the adstocked contribution with the direct contribution curve

Checklist

  • Checked that the pre-commit linting/style checks pass
    (some functions in delayed_saturated_mmm.py might need to be checked.
  • Included tests that prove the fix is effective or that the new feature works
    (have done some here, but not an exhaustive list of tests)
  • Added necessary documentation (docstrings and/or example notebooks)
    (only modified the mmm_example.ipynb file to show functionality. Would either need a prior predictive notebook or create an additional section in the file with more detailed description and appriopriate headers + placement.
  • If you are a pro: each commit corresponds to a relevant logical change

Sorry, but the commits are a bit all over the place 🙈

Modules affected

  • MMM
  • CLV

Type of change

  • New feature / enhancement
  • Bug fix
  • Documentation (A maybe here)
  • Maintenance
  • Other (please specify):

📚 Documentation preview 📚: https://pymc-marketing--477.org.readthedocs.build/en/477/

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@juanitorduz
Copy link
Collaborator

juanitorduz commented Jan 7, 2024

Thank you @eirikbaekkelund ! I will check this one in the upcoming weeks to give some initial feedback :)

@juanitorduz
Copy link
Collaborator

Why do we have so many commits from previous PRs/ Maybe you try rebasing?

Copy link

codecov bot commented Jan 7, 2024

Codecov Report

Attention: Patch coverage is 9.33333% with 136 lines in your changes missing coverage. Please review.

Project coverage is 85.05%. Comparing base (00c1509) to head (6a9638c).
Report is 157 commits behind head on main.

Current head 6a9638c differs from pull request most recent head 00b1289

Please upload reports for the commit 00b1289 to get more accurate results.

Files Patch % Lines
pymc_marketing/mmm/delayed_saturated_mmm.py 8.33% 88 Missing ⚠️
pymc_marketing/mmm/base.py 5.88% 48 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (00c1509) and HEAD (6a9638c). Click for more details.

HEAD has 1 upload more than BASE | Flag | BASE (00c1509) | HEAD (6a9638c) | |------|------|------| ||1|2|
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #477      +/-   ##
==========================================
- Coverage   90.82%   85.05%   -5.77%     
==========================================
  Files          21       21              
  Lines        1972     2121     +149     
==========================================
+ Hits         1791     1804      +13     
- Misses        181      317     +136     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@eirikbaekkelund eirikbaekkelund marked this pull request as draft January 25, 2024 14:59
@wd60622
Copy link
Contributor

wd60622 commented Jun 12, 2024

Hi @eirikbaekkelund
Is this something that you are still interested in?

@eirikbaekkelund
Copy link
Author

eirikbaekkelund commented Jun 15, 2024

@wd60622 sure, can get it done within the next 2-3 weeks i think with current availability

@eirikbaekkelund
Copy link
Author

still of interest for you guys @wd60622 @nialloulton @juanitorduz ?

@wd60622
Copy link
Contributor

wd60622 commented Jun 19, 2024

still of interest for you guys @wd60622 @nialloulton @juanitorduz ?

Hi @eirikbaekkelund
If you find it useful then I think it will be a great addition!
We did do a large refactor of the media transformations to each be a class themselves and no longer coupled with the MMM instance. i.e. media function agnostic MMM.
Because of these large changes, you will at least have to do a rebase or restart with these changes.
Here is an overview of the new class functionality which might be related to your edits. I am not familiar with your changes and would have to take a deeper look. Maybe you can make sense of what might still be missing.
If you share the produced curves from these edits, it might be easier to see and compare

@wd60622 wd60622 added the MMM label Jun 24, 2024
@wd60622
Copy link
Contributor

wd60622 commented Sep 7, 2024

Hi @eirikbaekkelund, still interested in this issue? If you are short on time, would you mind creating an issue for this feature? If you have a visual of what you are looking for, that would be helpful.

@eirikbaekkelund
Copy link
Author

eirikbaekkelund commented Oct 7, 2024

Hey @wd60622,

Apologies for the delayed response. I've been juggling a lot at work for some months, but I'm hoping to revisit this PR over the weekend. I'll need to familiarize myself with the latest updates to the library and clean up the boilerplate code.

I'm curious about your thoughts on creating a Streamlit app (similar to Desmos). If you're interested, let's schedule a quick call once I've made some progress.

EDIT:
I see the prior visualization has come as a feature, and ironically in streamlit! - great minds think alike :)
However, that seemed to be based of the priors but not in the way I designed it before. The idea is that prior samples are passed through whichever custom model you have to display the "support space" of the predictive likelihood. Should hopefully be pretty straight forward to build on that (famous last words)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants