pymc-marketing
pymc-marketing copied to clipboard
pymc-labs
Time varying intercept
This PR adds a time varying coefficient for the intercept. It's near-identical to #598, except it does not add support for time varying media contributions (this, we will add later).
Description
The API is now:
mmm = DelayedSaturatedMMM(
date_column=my_date_column_name,
channel_columns=my_media_column_names,
time_varying_intercept=True, # <-- This is new
)
Moreover, the model_config can now contain arguments for the time varying prior:
mmm = DelayedSaturatedMMM(
...
model_config={
...,
"tvp_kwargs": {
"m": 200,
"L": 100, # If left to None, is estimated from the data (supporting 1 year of future OOS prediction)
"eta_lam": 1,
"ls_mu": 52, # If left to None, is estimated from the data (supporting 1 year of future OOS prediction)
"ls_sigma": 10,
"cov_func": myCovFunc, # If left to None, default Matern52 is used (good default).
},
}
)
Changes are also made to the plotting and data setter functions, to support out of sample predictions.
Checklist
- [X] Checked that the pre-commit linting/style checks pass
- [ ] Included tests that prove the fix is effective or that the new feature works
- [X] Added necessary documentation (docstrings and/or example notebooks)
- [ ] If you are a pro: each commit corresponds to a relevant logical change
Modules affected
- [X] MMM
- [ ] CLV
Type of change
- [X] New feature / enhancement
- [ ] Bug fix
- [ ] Documentation
- [ ] Maintenance
- [ ] Other (please specify):
📚 Documentation preview 📚: https://pymc-marketing--628.org.readthedocs.build/en/628/
Check out this pull request on 
See visual diffs & provide feedback on Jupyter Notebooks.
Powered by ReviewNB
![review-notebook-app[bot] avatar](https://avatars.githubusercontent.com/in/15474?v=4 "Published by a pub.dev verified publisher"){kind=small}
Codecov Report
Attention: Patch coverage is 94.52055% with 4 lines in your changes are missing coverage. Please review.
Project coverage is 91.78%. Comparing base (
e63929a) to head (f0e59e6).
| Files | Patch % | Lines |
|---|---|---|
| pymc_marketing/mmm/base.py | 83.33% | 3 Missing :warning: |
| pymc_marketing/mmm/delayed_saturated_mmm.py | 93.33% | 1 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## main #628 +/- ##
==========================================
+ Coverage 91.73% 91.78% +0.05%
==========================================
Files 22 24 +2
Lines 2322 2386 +64
==========================================
+ Hits 2130 2190 +60
- Misses 192 196 +4
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
@ulfaslak can you please the notebook name in https://raw.githubusercontent.com/pymc-labs/pymc-marketing/main/docs/source/notebooks/index.md so that it gets passed tot he docs 🙏
@ulfaslak you might also wanna update the branch as we solved an issue in https://github.com/pymc-labs/pymc-marketing/pull/633 🙏
View / edit / reply to this conversation on ReviewNB
juanitorduz commented on 2024-04-16T10:42:09Z ----------------------------------------------------------------
can you add black lines so that the items are spaced correctly :)
View / edit / reply to this conversation on ReviewNB
juanitorduz commented on 2024-04-16T10:42:10Z ----------------------------------------------------------------
Would you mind adding
warnings.filterwarnings("ignore")
az.style.use("arviz-darkgrid")
plt.rcParams["figure.figsize"] = [12, 7]
plt.rcParams["figure.dpi"] = 100
%load_ext autoreload
%autoreload 2
%config InlineBackend.figure_format = "retina"
To make the style consistent :)
View / edit / reply to this conversation on ReviewNB
juanitorduz commented on 2024-04-16T10:42:10Z ----------------------------------------------------------------
- can you explain a bit what the
dooperator does for users who are not familiar with it? - Can you comment on the choice of the
true_params?Think about someone who just knows the concepts from the MMM example - add x and y labels to the plot :)
View / edit / reply to this conversation on ReviewNB
juanitorduz commented on 2024-04-16T10:42:11Z ----------------------------------------------------------------
Line #30. plt.title("Target")
can we use the fig, ax - plt.subplots() syntax?
View / edit / reply to this conversation on ReviewNB
juanitorduz commented on 2024-04-16T10:42:12Z ----------------------------------------------------------------
Line #7. sampler_config={"chains": 2, "draws": 10, "tune": 10, "nuts_sampler": "numpyro"},
Just 10 draws?
View / edit / reply to this conversation on ReviewNB
juanitorduz commented on 2024-04-16T10:42:13Z ----------------------------------------------------------------
Can you please give more explanation to these plots (titles, x and y labels ?) :D
Also, final words on the key takeaways :)
View / edit / reply to this conversation on ReviewNB
juanitorduz commented on 2024-04-16T10:42:12Z ----------------------------------------------------------------
Line #7. sampler_config={"chains": 2, "draws": 10, "tune": 10, "nuts_sampler": "numpyro"}, Just 10 draws?
ya that was for debugging. I'm revising this whole notebook and making it nice now :). Thanks for all the comments!
Alright, final tests added to cover all new cases. @wd60622 @juanitorduz curious to hear your thoughts on these, maybe I'm breaking some implicit conventions for how we structure tests, please let me know and I will correct.
Ahh there are merge conflicts. Will resolve, but tests can still be reviewed
Oh, also forgot to add @wd60622 s cool idea for wrapping TVP params in a class. Will add ASAP, but after (and if tests are fine in your opinions) we are probably good to merge
Just out of curiosity, have we tried to run the original notebook but now doing the intercept variable? I say this to know if we have seen the behaviour of the model in general with all parameters in place, controls, seasonality, channels, etc. 💡
Lots of comments, sorry. This is gonna be epic 🚀 Haven't touched the notebooks much but the plots are looking sweet
Really nice comments man, thanks a lot. Addressed them all. Will add some more tests, give it a good stare and then it should be close to the finish line.
@juanitorduz @wd60622 got the ✅ now.
Updates are:
- Addressed all of @wd60622 s last comments. Good stuff 🙌.
- I updated the deterministic returned by
time_varying_priorto be centered on one. This has some nice properties (see notebook). - I added tests for new logic that wasn't covered.
- I updated the notebook to include an example where time varying intercept is a trend (not a sinus).
View / edit / reply to this conversation on ReviewNB
juanitorduz commented on 2024-04-25T07:56:36Z ----------------------------------------------------------------
- Maybe write " time-varying parameters (TVP)" so that the user knows where "TVP" is coming from.
- Could you please add a comment (maybe below) on why we want it to be centered to have mean 1?
wd60622 commented on 2024-04-25T09:58:21Z ----------------------------------------------------------------
+1
View / edit / reply to this conversation on ReviewNB
juanitorduz commented on 2024-04-25T07:56:37Z ----------------------------------------------------------------
- Can you please add a title and ylabel to the plot? Also add a ";" so that we do not see the cell output :)
- Probably, we also want a legend for the meaning of the colors.
View / edit / reply to this conversation on ReviewNB
juanitorduz commented on 2024-04-25T07:56:38Z ----------------------------------------------------------------
Can you add more explanation on what you are doing and looking for? also, what are the takeaways from this plot?
View / edit / reply to this conversation on ReviewNB
juanitorduz commented on 2024-04-25T07:56:39Z ----------------------------------------------------------------
- Remove plt.show() and add ";" at the end.
ulfaslak commented on 2024-04-25T10:56:07Z ----------------------------------------------------------------
Isn't plt.show() more standard? I'll listen to a good argument (probably you're right).
juanitorduz commented on 2024-04-25T11:01:55Z ----------------------------------------------------------------
Well, it is not needed inside the notebook so that is the reason I suggest the change :)
ulfaslak commented on 2024-04-29T08:26:02Z ----------------------------------------------------------------
ChatGPT agrees with you! Did't know this trick, learned something, thanks.
View / edit / reply to this conversation on ReviewNB
juanitorduz commented on 2024-04-25T07:56:40Z ----------------------------------------------------------------
- Remove plt.show() and add ";" at the end.
View / edit / reply to this conversation on ReviewNB
juanitorduz commented on 2024-04-25T07:56:40Z ----------------------------------------------------------------
- So what is the conclusion? what does the user need to take when using this themselves?
- Add ";" at the end of the plot
View / edit / reply to this conversation on ReviewNB
juanitorduz commented on 2024-04-25T07:56:41Z ----------------------------------------------------------------
- Remove plt.show() and add ";" at the end.
View / edit / reply to this conversation on ReviewNB
juanitorduz commented on 2024-04-25T07:56:42Z ----------------------------------------------------------------
- Remove plt.show() and add ";" at the end.
View / edit / reply to this conversation on ReviewNB
juanitorduz commented on 2024-04-25T07:56:43Z ----------------------------------------------------------------
Do you think if you have such sales data in practice... given these results ... it is better to have a fixed intercept and add a variable of the form x -> log (x + 1) as a covariate so that we can have a better longer out-of-sample trend?
ulfaslak commented on 2024-04-29T09:00:21Z ----------------------------------------------------------------
Considering adding a third example with spiky events, cause really that's what this is good for, I think...
View / edit / reply to this conversation on ReviewNB
juanitorduz commented on 2024-04-25T07:56:43Z ----------------------------------------------------------------
- How long in the future do you expect the time-varying model to provide reasonable out-of-sample forecasts?
- Remove plt.show() and add ";" at the end.
cetagostini commented on 2024-04-27T23:10:15Z ----------------------------------------------------------------
Agree with Juan here, I think it would be good to mention here what was discussed about the period within which the TVP does not deviate too much from reality. Being 90 days a great threshold if you work with daily data, of course not a consistent law for all cases, but regularly see it (Even in this example).
View / edit / reply to this conversation on ReviewNB
juanitorduz commented on 2024-04-25T07:56:44Z ----------------------------------------------------------------
add ";" at the end
@ulfaslak This looks fantastic! The code itself is ✅ IMO (any comments @wd60622 ?). On the other hand, I think the notebook needs a bit more of the storyline. Consider a user who has never seen a time-varying intercept model. The main points a reader should get from the notebook (IMO) are:
- What is a TV intercept?
- Why is it useful?
- How to use it in PyMC-Marketing?
- What are the benefits? (thinking about the MMM objective of better inference)
- When to use it?
- When it might not work?
Some of these questions have already been answer in the notebook. I think we just meet to complete the storyline and the takeaways 🤜🤛
WDYT?
PS. Also I added some minor comments about the plots XD