Handle new data correctly and extend functionality of MMM posterior predictive methods #482

williambdean · 2024-01-11T19:30:27Z

Description

Redo of the support for new mmm data

This brings in the feedback about

having the include_last_observations for all predict methods with X_pred
Support for original scale of target with the original_scale keyword that can be passed to all predict methods

Related Issue

Closes scaling control vars #472 sample_posterior_predictive doesn't scale X data #450 _data_setter in MMM misses date and fourier data #400 Value Error on DelayedSaturatedMMM predict() for test data #396
Related to Support New Data for MMM model #444 Out-of-Sample Predictions #392

Checklist

Checked that the pre-commit linting/style checks pass
Included tests that prove the fix is effective or that the new feature works
Added necessary documentation (docstrings and/or example notebooks)
If you are a pro: each commit corresponds to a relevant logical change

Modules affected

MMM
CLV

Type of change

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

tests/mmm/test_delayed_saturated_mmm.py

williambdean · 2024-01-11T20:16:37Z

Introducing the new predict_posterior call on mmm model

**This is just an arbitrary fit with mmm-example.csv with last 10 weeks (rows) held out

codecov · 2024-01-11T22:46:54Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (49e1e80) 90.83% compared to head (bd44767) 91.12%.
Report is 5 commits behind head on main.

Files	Patch %	Lines
pymc_marketing/mmm/delayed_saturated_mmm.py	95.12%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #482      +/-   ##
==========================================
+ Coverage   90.83%   91.12%   +0.29%     
==========================================
  Files          21       21              
  Lines        1974     2018      +44     
==========================================
+ Hits         1793     1839      +46     
+ Misses        181      179       -2

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

pymc_marketing/mmm/delayed_saturated_mmm.py

ricardoV94 · 2024-01-12T07:45:04Z

pymc_marketing/mmm/delayed_saturated_mmm.py

+        with self.model:  # sample with new input data
+            post_pred = pm.sample_posterior_predictive(self.idata, **kwargs)
+            if extend_idata:
+                self.idata.extend(post_pred)  # type: ignore


You need to define join otherwise calling this method twice will ignore the second run

See pymc-devs/pymc-extras#249

This applies to prior and fit as well, but maybe it's a good time as any to fix it as it's a pretty serious one.

I had a PR on this but haven't come back to it

Sounds good. This is in the method override. Shall I add to the model_builder method as well?

Yup. Feel free to reuse as much as you can from the PR of the pymc-experimental ModelBuilder (thinking about tests as well). Just add the author as a co-author or mention in the commit if you do that.

Sounds good. I will dive into it more later

I've just added a check that the idata will be the first sample from two method calls to keep it simple. Hope that's what you expected

I think fit is still overriding whatever was there before. Example sample_prior_predictive -> fit removes sample_prior_predictive (if I read the code correctly)

Doesn't need to be done in this PR, just mentioning here for the related issue #459

juanitorduz

This looks super nice! Thanks! I just added some comments about the docs and other minor things.

pymc_marketing/mmm/delayed_saturated_mmm.py

juanitorduz · 2024-01-15T21:16:40Z

@wd60622 After we merge this one, we need to add (or create) an example to the docs (let's create an issue). This one will be a game-changer feature 🚀

pymc_marketing/mmm/delayed_saturated_mmm.py

tests/mmm/test_delayed_saturated_mmm.py

tests/model_builder/test_model_builder.py

williambdean · 2024-01-16T21:06:33Z

Thank you guys for the review! I will have some time tomorrow to make some changes

williambdean · 2024-01-17T16:16:05Z

I've made edits based on all the comments @juanitorduz and @ricardoV94
Thank you for the feedback

juanitorduz

This is amazing @wd60622 ! From my side, I just wanna push this out and get feedback. My sleep deprivation (👶 ) might make me miss some details so if @ricardoV94 is happy I suggest we merge and release 0.4 🚀

tests/mmm/test_delayed_saturated_mmm.py

tests/model_builder/test_model_builder.py

pymc_marketing/model_builder.py

pymc_marketing/mmm/delayed_saturated_mmm.py

williambdean · 2024-01-18T16:37:30Z

Hi @ricardoV94. Thanks for the feedback. I just went through all of it and made some changes.

For the xarray tests, I changed to used check for the second sampling result and am using xr.testing module now for more robust checks.

juanitorduz · 2024-01-24T08:50:31Z

LGTM 🚀 ! Let's wait for @ricardoV94 's feedback :)

ricardoV94 · 2024-01-25T12:33:58Z

@wd60622 everything looks kosher, but there is still this open discussion: #482 (comment)

Given how all sampling/fitting methods require x/y anyway I changed my mind and I don't think the _reset_data call is needed after all? The less magic probably the better

williambdean · 2024-01-25T16:07:54Z

@wd60622 everything looks kosher, but there is still this open discussion: #482 (comment)

Given how all sampling/fitting methods require x/y anyway I changed my mind and I don't think the _reset_data call is needed after all? The less magic probably the better|

Perfect. I've changed that back so that the data will not be reset

tests/mmm/test_delayed_saturated_mmm.py

juanitorduz · 2024-01-25T17:23:41Z

Are we ready to merge? 🤞

williambdean · 2024-01-25T17:33:35Z

Are we ready to merge? 🤞

Had the affected tests passing locally. They seem to be waiting to start on GitHub 😢

juanitorduz · 2024-01-25T18:15:10Z

It's green! 💚. I think we just need @ricardoV94 's last approval :)

ricardoV94 · 2024-01-25T20:29:05Z

Merged, any notebooks that need updating?

juanitorduz · 2024-01-25T20:32:42Z

Merged, any notebooks that need updating?

I ran the notebook in my first review and it worked perfectly. I can check tomorrow (@wd60622 do we need an update?).

williambdean · 2024-01-25T22:21:26Z

Firstly, thank you both for the reviews!

Merged, any notebooks that need updating?

I ran the notebook in my first review and it worked perfectly. I can check tomorrow (@wd60622 do we need an update?).

I haven't touched the notebooks. I would like to add an example in docstring / notebook with #494
I will check get to that on the weekend

run pre-commit

a9bb925

williambdean commented Jan 11, 2024

View reviewed changes

tests/mmm/test_delayed_saturated_mmm.py Outdated Show resolved Hide resolved

williambdean changed the title ~~run pre-commit~~ Support New MMM Data Jan 11, 2024

williambdean mentioned this pull request Jan 11, 2024

Support New Data for MMM model #444

Closed

williambdean added 2 commits January 11, 2024 21:14

more accurate description of the kwargs

b6f9dbb

split out the tests

4c773c6

juanitorduz self-requested a review January 11, 2024 21:28

remove since numpy

96567ef

williambdean mentioned this pull request Jan 12, 2024

New spends forward pass #456

Merged

ricardoV94 reviewed Jan 12, 2024

View reviewed changes

williambdean added 3 commits January 12, 2024 09:25

rename output_var to y and join right while extend

ad62f6d

use the property when accessed

66aaf3a

test for the join=right

6dda56a

juanitorduz requested changes Jan 15, 2024

View reviewed changes

ricardoV94 reviewed Jan 16, 2024

View reviewed changes

pymc_marketing/mmm/delayed_saturated_mmm.py Show resolved Hide resolved

tests/mmm/test_delayed_saturated_mmm.py Show resolved Hide resolved

tests/mmm/test_delayed_saturated_mmm.py Show resolved Hide resolved

tests/model_builder/test_model_builder.py Outdated Show resolved Hide resolved

williambdean added 2 commits January 17, 2024 16:55

extract the transformation and test

3d61f38

test constraints on the data

72b8b86

remove the arg that isnt used

c5d72ab

juanitorduz approved these changes Jan 17, 2024

View reviewed changes

ricardoV94 mentioned this pull request Jan 18, 2024

idata is removed if fit is called #459

Closed

ricardoV94 reviewed Jan 18, 2024

View reviewed changes

tests/mmm/test_delayed_saturated_mmm.py Outdated Show resolved Hide resolved

ricardoV94 requested changes Jan 18, 2024

View reviewed changes

tests/model_builder/test_model_builder.py Outdated Show resolved Hide resolved

ricardoV94 changed the title ~~Support New MMM Data~~ Handle new data correctly in MMM posterior predictive methods Jan 18, 2024

ricardoV94 added bug Something isn't working MMM labels Jan 18, 2024

ricardoV94 added the enhancement New feature or request label Jan 18, 2024

ricardoV94 changed the title ~~Handle new data correctly in MMM posterior predictive methods~~ Handle new data correctly and extend functionality of MMM posterior predictive methods Jan 18, 2024

williambdean commented Jan 18, 2024

View reviewed changes

pymc_marketing/model_builder.py Show resolved Hide resolved

pymc_marketing/model_builder.py Outdated Show resolved Hide resolved

ricardoV94 reviewed Jan 18, 2024

View reviewed changes

pymc_marketing/mmm/delayed_saturated_mmm.py Outdated Show resolved Hide resolved

williambdean added 3 commits January 18, 2024 17:18

change back the accidental test match

839c666

make consistent extend_idata default

cdceeb8

test second sample stays

6ec2789

juanitorduz requested a review from ricardoV94 January 18, 2024 19:35

williambdean mentioned this pull request Jan 22, 2024

Document MMM out of sample prediction #494

Closed

dont set back data

2c2a7c6

ricardoV94 reviewed Jan 25, 2024

View reviewed changes

tests/mmm/test_delayed_saturated_mmm.py Outdated Show resolved Hide resolved

remove explicit check of set back

bd44767

ricardoV94 approved these changes Jan 25, 2024

View reviewed changes

ricardoV94 merged commit bc71f15 into pymc-labs:main Jan 25, 2024

williambdean deleted the support-new-data-redo branch January 25, 2024 22:18

williambdean mentioned this pull request Jan 31, 2024

Unable to predict on unseen data due to need to change coords in data setter #507

Closed

PabloRoque mentioned this pull request Sep 5, 2024

Unable to scale in sample_posterior_predictive with var_names other than target #982

Closed

Handle new data correctly and extend functionality of MMM posterior predictive methods #482

Handle new data correctly and extend functionality of MMM posterior predictive methods #482

Uh oh!

Conversation

williambdean commented Jan 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Checklist

Modules affected

Type of change

Uh oh!

Uh oh!

williambdean commented Jan 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

ricardoV94 Jan 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ricardoV94 Jan 12, 2024

Choose a reason for hiding this comment

Uh oh!

williambdean Jan 12, 2024

Choose a reason for hiding this comment

Uh oh!

ricardoV94 Jan 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

williambdean Jan 12, 2024

Choose a reason for hiding this comment

Uh oh!

williambdean Jan 12, 2024

Choose a reason for hiding this comment

Uh oh!

ricardoV94 Jan 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ricardoV94 Jan 18, 2024

Choose a reason for hiding this comment

Uh oh!

juanitorduz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

juanitorduz commented Jan 15, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

williambdean commented Jan 16, 2024

Uh oh!

williambdean commented Jan 17, 2024

Uh oh!

juanitorduz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

williambdean commented Jan 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

juanitorduz commented Jan 24, 2024

Uh oh!

williambdean commented Jan 11, 2024 •

edited

Loading

williambdean commented Jan 11, 2024 •

edited

Loading

codecov bot commented Jan 11, 2024 •

edited

Loading

ricardoV94 Jan 12, 2024 •

edited

Loading

ricardoV94 Jan 12, 2024 •

edited

Loading

ricardoV94 Jan 18, 2024 •

edited

Loading

williambdean commented Jan 18, 2024 •

edited

Loading

juanitorduz commented Jan 25, 2024 •

edited

Loading