Add time-varying coefficient #598

ulfaslak · 2024-03-19T12:55:17Z

This PR adds time-varying coefficient options to the DelayedSaturatedMMM class.

Description

Specifically the following model specification is now possible:

mmm = DelayedSaturatedMMM(
    date_column=my_date_column_name,
    channel_columns=my_media_column_names,
    time_varying_intercept=True,            # <-- These two args
    time_varying_media_effect=True,         # <-- are what's up
)

time_varying_intercept creates a time-varying prior on the intercept, while time_varying_media_effect creates a time-varying prior on the total media contribution (i.e. not individual columns, this will be added in a later PR).

🚨 For now let's get some reviews on this and agree what is a good way to add this functionality. Then we can add tests and docs later.

To make this easier to review, maybe start looking inside the mmm_tvp_example.ipynb, then check out tvp.py, and then go over the modifications in related filed I had to make to get the API changes to work.

Related Issue

Related to Time-varying coefficients #231 Add HSGP prior #415

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--598.org.readthedocs.build/en/598/

review-notebook-app · 2024-03-19T12:55:22Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

wd60622

Just some initial thoughts. will keep an eye out on changes

pymc_marketing/mmm/tvp.py

pymc_marketing/mmm/delayed_saturated_mmm.py

wd60622 · 2024-03-19T15:19:30Z

resolved the conflict to see the status of the tests. @ulfaslak

pre-commit stuff 🥲

codecov · 2024-03-19T15:31:52Z

Codecov Report

Attention: Patch coverage is 15.78947% with 48 lines in your changes are missing coverage. Please review.

Project coverage is 34.77%. Comparing base (f055f98) to head (dac69da).
Report is 5 commits behind head on main.

❗ Current head dac69da differs from pull request most recent head ec316b4. Consider uploading reports for the commit ec316b4 to get more accurate results

Files	Patch %	Lines
pymc_marketing/mmm/delayed_saturated_mmm.py	9.09%	20 Missing ⚠️
pymc_marketing/mmm/tvp.py	20.00%	16 Missing ⚠️
pymc_marketing/mmm/base.py	0.00%	11 Missing ⚠️
pymc_marketing/mmm/utils.py	75.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #598       +/-   ##
===========================================
- Coverage   91.79%   34.77%   -57.03%     
===========================================
  Files          22       22               
  Lines        2267     2220       -47     
===========================================
- Hits         2081      772     -1309     
- Misses        186     1448     +1262

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

cetagostini-wise · 2024-03-19T15:35:11Z

pymc_marketing/mmm/delayed_saturated_mmm.py

@@ -34,6 +36,8 @@ def __init__(
        date_column: str,
        channel_columns: List[str],
        adstock_max_lag: int,
+        time_varying_media_effect: bool = False,
+        time_varying_intercept: bool = False,


I think it would be great if we could control the HSGP. Could we add an hsgp_config similar to how we defined the priors? So the more "expert" user could play with how much flexibility they want to give to the HSGP?

Maybe better to just support that in the model config? The chief thing you'd want to control is the covariance function, m and L. But for MMM this can even be simplified to just lengthscale and m, I believe since optimal L can be calculated form the length of the data and the lengthscale.

cetagostini · 2024-03-19T15:39:10Z

pymc_marketing/mmm/delayed_saturated_mmm.py

+                    name="intercept", **self.model_config["intercept"]["kwargs"]
+                )
+
+            if self.time_varying_media_effect:


Instead of checking twice, would it be better to check by the end? You load the data and apply the multiplier only if True all at once, saving a few lines of code.

Yes. Can I refactor this whole function actually? Fingertips itching.

cetagostini · 2024-03-19T15:41:03Z

pymc_marketing/mmm/tvp.py

+        f = phi @ (hsgp_coefs * sqrt_psd).T
+        if positive:
+            f = softplus(f)
+        return pm.Deterministic(name, f, dims=dims)


I think would be great to create a plot for the varying parameters, showing the recovered latent pattern which is affecting the channels and/or the intercept, what do you think?

Agree. Will add!

cetagostini · 2024-03-19T15:47:32Z

pymc_marketing/mmm/delayed_saturated_mmm.py

+
+            channel_contributions_var = channel_adstock_saturated * beta_channel
+            if self.time_varying_media_effect:
+                channel_contributions_var *= tv_multiplier_media[:, None]


It is not a strong opinion, but I think that if we are using the logic of a base contribution, which is then modified, it would be great to have this join saved in another Deterministic variable. So, users can understand their BASE contribution and the latent multiplier effect independently. We would need to have a small extra piece of code, but what do you think?

e.g:

pm.Deterministic( var = channel_contributions_var * tv_multiplier_media[:, None] name='varying_contribution' #or something like it )

Yeah this is a nice idea. I think I might actually change the time_varying_prior function so it always swings in the positive range. Then when using it, it always works as a multiplier on base contributions. Will also make joining this logic with logic for hierarchical parameters easy.

I'll make this change, it's nice.

cetagostini · 2024-03-19T15:52:53Z

pymc_marketing/mmm/delayed_saturated_mmm.py

-                dims=("date", "channel"),
+                name="channel_contributions", 
+                var=channel_contributions_var, 
+                dims=("date", "channel"), 
            )

            mu_var = intercept + channel_contributions.sum(axis=-1)


Since the HSGP is in dimension "date" from the beginning, I feel that we are first transforming said HSGP to a form that is compatible with ("date", "channel") to return at the end to "date" only. Isn't it better to transform the channels to "date" and finally multiply by the HSGP without needing of [:,None]?

It's just an opinion, I think it would be more appropriate the other way and we can avoid a somewhat unnecessary transformation, what do you think?

Don't fully understand. You can just commit the change if you believe it is best.

In general this time_varying_prior function supports two dimensions but I'm only using the 1D case here, maybe that's confusing.

I should actually add support for individually varying parameters in this PR too... not complicated

I should actually add support for individually varying parameters in this PR too... not complicated
I suggest we keep the scope of this PR small and then iterate

cetagostini · 2024-03-19T15:54:09Z

pymc_marketing/mmm/tvp.py

+
+    with pm.modelcontext(model) as model:
+        if cov_func is None:
+            eta = pm.Exponential(f"eta_{name}", lam=eta_lam)


I think using this current implementation, we could move these distributions into the model_config?

oh this would be really clever, it just becomes another supported distribution?

that's better than an hsgp_config I think. I'm trying to work out what's the right level of rigidity to build into this thing. There are some things here you wouldn't want to change as a user I think. And maybe this is just me, but as we put distributions into the config, we pay the price of more obscure code. Opinions?

It would be nice to allow these priors to be defined in the config as GPs are quite sensitive to priors.

cetagostini · 2024-03-19T15:57:17Z

Hey hey, amazing work! I made a few comments, but all are initial thoughts. I'll deep dive during the week 💪🏻

ulfaslak · 2024-03-20T08:40:20Z

Hey hey, amazing work! I made a few comments, but all are initial thoughts. I'll deep dive during the week 💪🏻

This is super nice thanks @cetagostini!

twiecki · 2024-04-03T12:55:09Z

pymc_marketing/mmm/utils.py

@@ -329,6 +331,10 @@ def apply_sklearn_transformer_across_dim(

    return data

+
+def softplus(x: pt.TensorVariable) -> pt.TensorVariable:


I think this exists in pytensor.tensor.math.

…varying-prior

juanitorduz · 2024-04-08T07:30:35Z

pymc_marketing/mmm/base.py

@@ -336,19 +342,26 @@ def plot_posterior_predictive(
            )

            ax.fill_between(
-                x=self.X[self.date_column],
+                x=posterior_predictive_data.date,


What is the reason for this change? The date column can have a different name.

This enables OOS posterior predictive. self.X does not get updated upon self._data_setter(X_new). So if the posterior predictive is for new OOS data, then self.X[self.date_column] will still be the in-sample dates.

juanitorduz · 2024-04-08T07:31:39Z

pymc_marketing/mmm/base.py

            )
+
+            assert len(target_to_plot) == len(posterior_predictive_data.date), (


can wee keep date_col as generic date name?

You mean set date_col = posterior_predictive_data.date somewhere earlier in this function?

juanitorduz · 2024-04-08T07:32:59Z

pymc_marketing/mmm/delayed_saturated_mmm.py

@@ -28,6 +29,7 @@

 __all__ = ["DelayedSaturatedMMM"]

+DAYS_IN_YEAR = 365.25


We can move this into a constants.py file :)

Perfect. Will do.

juanitorduz · 2024-04-08T07:35:51Z

pymc_marketing/mmm/delayed_saturated_mmm.py

+                    X_mid=self._time_index_mid,
+                    positive=True,
+                    m=200,
+                    L=[self._time_index_mid + DAYS_IN_YEAR / self._time_resolution],


I think it might be cleaner to use the c parameeer?

c: float The proportion extension factor. Used to construct L from X. Defined as S = max|X| such that X is in [-S, S]. L is calculated as c * S. One of c or L must be provided. Further information can be found in Ruitort-Mayol et al.

Unfortunately this causes some trouble with predicting out of sample. @bwengals did you find the cause for this?

juanitorduz · 2024-04-08T07:43:18Z

pymc_marketing/mmm/tvp.py

+        else:
+            hsgp_size = m
+        gp = pm.gp.HSGP(m=[m], L=[L], cov_func=cov_func)
+        phi, sqrt_psd = gp.prior_linearized(Xs=X[:, None] - X_mid)


Note for future review: We need to be careful we center the data with respect to the training set (even when we are doing out of sample prediction)

See doctstrings in https://github.com/pymc-devs/pymc/blob/main/pymc/gp/hsgp_approx.py#L243

Can you elaborate? What is the concern?

juanitorduz

Added some small comments/ questions fot thie first round :)

juanitorduz · 2024-04-08T07:47:11Z

@ulfaslak could you please rebase from main as the tests are failing because #615 🙏 ?

review-notebook-app · 2024-04-08T07:49:43Z

View / edit / reply to this conversation on ReviewNB

juanitorduz commented on 2024-04-08T07:49:42Z
----------------------------------------------------------------

Line #8.        sampler_config={"nuts_sampler": "numpyro", "target_accept": 0.98},

I such large target_accept required?

cetagostini-wise · 2024-04-08T07:50:11Z

pymc_marketing/mmm/delayed_saturated_mmm.py

@@ -38,6 +40,8 @@ def __init__(
        date_column: str,
        channel_columns: List[str],
        adstock_max_lag: int,
+        time_varying_media_effect: bool = False,


Hey, small comment here... This is a matter of taste but I think having two parameters for the same component is a bit strange. What do you think if we evaluate with strings or tuples?

time_varying = ('intercept', 'media') #or

time_varying = 'intercept-media'

I think it would be better inside for the API

I quite like this. Maybe this gets too complicated but:

time_varying='intercept' ... time_varying='total_media' ... time_varying=['intercept', 'total_media'] ... time_varying=['intercept', 'channel1', 'channe2']

could work

Maybe even

time_varying=['intercept', ('channel1', 'channel2')] # now channel1 and channel2 are summed and multiplied by a time varying coef

juanitorduz · 2024-04-08T07:53:55Z

@ulfaslak This feature is very exciting! As this PR was about collecting feedback, I have a proposal: Split this PR into two:

Time-varying intercept
Time-varying media (as in this PR)

The reason? to make The review process simpler and faster :)

For each PR, we should aim for:

Implementation with changes just on the scope of the PR.
Docstring
Tests
Example Nb

This way we can iterate fast, as probably we wanna push this one out soon :) 🚀

The main question from what i see is if we wanna provide all the priors and HSGP parameters configurable via model_config (Personally, i think we should)

nialloulton · 2024-04-08T07:55:40Z

Yep, that approach makes sense! Get Outlook for iOS<https://aka.ms/o0ukef>

…

________________________________ From: Juan Orduz ***@***.***> Sent: Monday, April 8, 2024 8:54:18 AM To: pymc-labs/pymc-marketing ***@***.***> Cc: Niall Oulton ***@***.***>; Review requested ***@***.***> Subject: Re: [pymc-labs/pymc-marketing] Add time-varying coefficient (PR #598) @ulfaslak<https://github.com/ulfaslak> This feature is very exciting! As this PR was about collecting feedback, I have a proposal: Split this PR into two: 1. Time-varying intercept 2. Time-varying media (as in this PR) The reason? to make The review process simpler and faster :) For each PR, we should aim for: * Implementation with changes just on the scope of the PR. * Docstring * Tests * Example Nb this way we can iterate fast as probably we wanna push this one out soon :) 🚀 — Reply to this email directly, view it on GitHub<#598 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/A5SZLZLQH2TTLVLRPAQ64B3Y4JEKVAVCNFSM6AAAAABE5QDY2WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBSGA4TENZVHE>. You are receiving this because your review was requested.Message ID: ***@***.***>

cetagostini-wise · 2024-04-08T08:01:52Z

Following @juanitorduz mention, and perhaps to align with #602 , if it's worthy.

We could create a models.components folder and a time.py file to save the implementation? This could help to add and save all code related to "time" dependencies.

What about if we treat this implementation as components as well? Similar to the PR attach (#602 ), all time varying media or intercept happens in a class which then is added to the main model.

The split between two PRs (media/intercept) and the component base implementation, should make everything very easy to debug, test and play. What everyone thinks? @wd60622 @ulfaslak

cetagostini-wise · 2024-04-08T08:02:41Z

The main question from what i see is if we wanna provide all the priors and HSGP parameters configurable via model_config (Personally, i think we should)

@juanitorduz My personal opinion on it, is we should allow it.

juanitorduz · 2024-04-08T08:04:49Z

Following @juanitorduz mention, and perhaps to align with #602 , if it's worthy.

We could create a models.components folder and a time.py file to save the implementation? This could help to add and save all code related to "time" dependencies.

What about if we treat this implementation as components as well? Similar to the PR attach, all time varying media or intercept happens in a class which then is added to the main model. This and the split between two PRs (media/intercept) could make everything very easy to debug, test and play. What everyone thinks? @wd60622 @ulfaslak

The more modular, the better! Let's work on smaller PR so that we can iterate faster :)

ulfaslak · 2024-04-08T09:41:52Z

@ulfaslak This feature is very exciting! As this PR was about collecting feedback, I have a proposal: Split this PR into two:

Time-varying intercept

Time-varying media (as in this PR)

The reason? to make The review process simpler and faster :)

For each PR, we should aim for:

Implementation with changes just on the scope of the PR.

Docstring

Tests

Example Nb

This way we can iterate fast, as probably we wanna push this one out soon :) 🚀

The main question from what i see is if we wanna provide all the priors and HSGP parameters configurable via model_config (Personally, i think we should)

I like this.

I will try to bang the first PR together then. Thanks for a lot of high-quality feedback 👌

ulfaslak · 2024-04-08T12:21:36Z

@ulfaslak could you please rebase from main as the tests are failing because #615 🙏 ?

I will close this and make a new PR for intercept-tvp on a branch off of main.

Add time-varying prior functionality to DelayedSaturatedMMM

faaba0f

ulfaslak requested review from bwengals, wd60622, cetagostini and nialloulton March 19, 2024 12:55

wd60622 reviewed Mar 19, 2024

View reviewed changes

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

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

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

ulfaslak and others added 2 commits March 19, 2024 15:53

resolve wd's comments

3ce5ac4

Merge branch 'main' into time-varying-prior

dac69da

cetagostini-wise reviewed Mar 19, 2024

View reviewed changes

cetagostini reviewed Mar 19, 2024

View reviewed changes

juanitorduz added enhancement New feature or request MMM labels Mar 24, 2024

twiecki reviewed Apr 3, 2024

View reviewed changes

Merge branch 'main' of github.com:pymc-labs/pymc-marketing into time-…

ec316b4

…varying-prior

juanitorduz reviewed Apr 8, 2024

View reviewed changes

cetagostini-wise reviewed Apr 8, 2024

View reviewed changes

juanitorduz added priority: high request discussion labels Apr 8, 2024

ulfaslak closed this Apr 8, 2024

ulfaslak mentioned this pull request Apr 15, 2024

Time varying intercept #628

Merged

11 tasks

		@@ -329,6 +331,10 @@ def apply_sklearn_transformer_across_dim(

		return data


		def softplus(x: pt.TensorVariable) -> pt.TensorVariable:

		)

		assert len(target_to_plot) == len(posterior_predictive_data.date), (

		@@ -28,6 +29,7 @@

		__all__ = ["DelayedSaturatedMMM"]

		DAYS_IN_YEAR = 365.25

Add time-varying coefficient #598

Add time-varying coefficient #598

Conversation

ulfaslak commented Mar 19, 2024 • edited Loading

Description

Related Issue

Checklist

Modules affected

Type of change

review-notebook-app bot commented Mar 19, 2024

wd60622 left a comment

Choose a reason for hiding this comment

wd60622 commented Mar 19, 2024 • edited Loading

codecov bot commented Mar 19, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cetagostini commented Mar 19, 2024

ulfaslak commented Mar 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanitorduz left a comment

Choose a reason for hiding this comment

juanitorduz commented Apr 8, 2024

review-notebook-app bot commented Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanitorduz commented Apr 8, 2024 • edited Loading

nialloulton commented Apr 8, 2024 via email

cetagostini-wise commented Apr 8, 2024 • edited Loading

cetagostini-wise commented Apr 8, 2024

juanitorduz commented Apr 8, 2024

ulfaslak commented Apr 8, 2024

ulfaslak commented Apr 8, 2024 • edited Loading

ulfaslak commented Mar 19, 2024 •

edited

Loading

wd60622 commented Mar 19, 2024 •

edited

Loading

codecov bot commented Mar 19, 2024 •

edited

Loading

review-notebook-app bot commented Apr 8, 2024 •

edited

Loading

juanitorduz commented Apr 8, 2024 •

edited

Loading

cetagostini-wise commented Apr 8, 2024 •

edited

Loading

ulfaslak commented Apr 8, 2024 •

edited

Loading