Skip to content

Multiplicative MMM (Log and Log-Log Models)#2477

Draft
juanitorduz wants to merge 25 commits into
mainfrom
multiplicative-mmm
Draft

Multiplicative MMM (Log and Log-Log Models)#2477
juanitorduz wants to merge 25 commits into
mainfrom
multiplicative-mmm

Conversation

@juanitorduz
Copy link
Copy Markdown
Collaborator

@juanitorduz juanitorduz commented Apr 2, 2026

Experimental API for multiplicative MMMs.


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

@cursor
Copy link
Copy Markdown
Contributor

cursor Bot commented Apr 2, 2026

PR Summary

Medium Risk
Touches core MMM model-building, deterministics used by budget optimization, and contribution decomposition logic; mistakes could silently change reported contributions/optimization objectives, though changes are gated behind link="log" with added validation/tests.

Overview
Adds first-class support for multiplicative MMMs via a new link parameter (identity vs log) backed by a LinkSpec strategy (pymc_marketing/mmm/link.py). For link="log", the model now defaults to a LogNormal likelihood, enforces strictly-positive targets, warns when mu_effects are used (changed semantics), and redefines total_media_contribution_original_scale as a counterfactual lift (also adding y_hat_original_scale).

Updates contribution/decomposition APIs to be link-aware: MMM.compute_mean_contributions_over_time() and MMMIDataWrapper.get_contributions() use a hybrid decomposition under log link (counterfactual total media lift with proportional per-channel allocation) and disables add_original_scale_contribution_variable for log-link models. Adds LogSaturation (beta * log1p(x)) to enable log-log specifications, wires it into exports/serialization, and introduces a focused test suite (tests/mmm/test_link.py) covering validation, deterministics, decomposition, and compatibility guardrails; includes a design doc (multiplicative_models.md).

Written by Cursor Bugbot for commit a4fff27. This will update automatically on new commits. Configure here.

Comment thread pymc_marketing/mmm/components/saturation.py
Comment thread pymc_marketing/data/idata/mmm_wrapper.py Outdated
@juanitorduz juanitorduz marked this pull request as draft April 2, 2026 08:36
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 2, 2026

Codecov Report

❌ Patch coverage is 95.74468% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.91%. Comparing base (de4a3f8) to head (158a639).

Files with missing lines Patch % Lines
pymc_marketing/mmm/mmm.py 91.37% 5 Missing ⚠️
pymc_marketing/mmm/link.py 95.83% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2477      +/-   ##
==========================================
+ Coverage   93.86%   93.91%   +0.05%     
==========================================
  Files          90       92       +2     
  Lines       13959    14127     +168     
==========================================
+ Hits        13102    13267     +165     
- Misses        857      860       +3     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@review-notebook-app
Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@github-actions github-actions Bot added the docs Improvements or additions to documentation label Apr 2, 2026
@juanitorduz
Copy link
Copy Markdown
Collaborator Author

@BugBot review

Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

@juanitorduz
Copy link
Copy Markdown
Collaborator Author

@BugBot review

Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Restore legacy MMM scaling and serialization logic in mmm.py to match main so this file drops out of the PR diff.

Made-with: Cursor
@review-notebook-app
Copy link
Copy Markdown

review-notebook-app Bot commented Apr 8, 2026

View / edit / reply to this conversation on ReviewNB

ErikRingen commented on 2026-04-08T09:50:47Z
----------------------------------------------------------------

I'm concerned that, in the log-log model, \beta will be quite far from an elasticity because the input spend (x) is scaled by its max, and therefore the log1p(x) will in practice never be a good approximation of log(x).


@review-notebook-app
Copy link
Copy Markdown

review-notebook-app Bot commented Apr 8, 2026

View / edit / reply to this conversation on ReviewNB

ErikRingen commented on 2026-04-08T09:50:48Z
----------------------------------------------------------------

works for the mean contribution due to linearity of expectations, but I think it would prevent confusion to instead write the contribution as the expectation of the difference because (1) that is what is actually done in log_counterfactual_remove_component (computes diff then summarizes using mean) and (2) it will still be accurate for the credible intervals of the contributions.


@review-notebook-app
Copy link
Copy Markdown

review-notebook-app Bot commented Apr 8, 2026

View / edit / reply to this conversation on ReviewNB

ErikRingen commented on 2026-04-08T09:50:48Z
----------------------------------------------------------------

I find it a bit confusing to call the intercept a "residual" scaling factor. Also, while I get the maths of defining the intercept contribution this way, would it be more natural/in-line with how users want to interpret the contribution this to define it as: exp(a) * s?


@review-notebook-app
Copy link
Copy Markdown

review-notebook-app Bot commented Apr 8, 2026

View / edit / reply to this conversation on ReviewNB

ErikRingen commented on 2026-04-08T09:50:49Z
----------------------------------------------------------------

This is not correct: "Note: Under the default scaling pipeline, channel data is divided by channel_scale, so these are approximate elasticities with respect to scaled spend rather than strict textbook elasticities.". Elasticites are dimensionless, so the choice of scaling impacts the intercept but not the \beta


@review-notebook-app
Copy link
Copy Markdown

review-notebook-app Bot commented Apr 8, 2026

View / edit / reply to this conversation on ReviewNB

ErikRingen commented on 2026-04-08T09:50:50Z
----------------------------------------------------------------

Line #22.    fig.suptitle("None-Media Contributions", fontsize=18, fontweight="bold");

"Non-Media contributions"


@ErikRingen
Copy link
Copy Markdown
Contributor

View / edit / reply to this conversation on ReviewNB

ErikRingen commented on 2026-04-08T09:50:47Z ----------------------------------------------------------------

I'm concerned that, in the log-log model, \beta will be quite far from an elasticity because the input spend (x) is scaled by its max, and therefore the log1p(x) will in practice never be a good approximation of log(x).

I think the simplest resolution

View / edit / reply to this conversation on ReviewNB

ErikRingen commented on 2026-04-08T09:50:48Z ----------------------------------------------------------------

I find it a bit confusing to call the intercept a "residual" scaling factor. Also, while I get the maths of defining the intercept contribution this way, would it be more natural/in-line with how users want to interpret the contribution this to define it as: exp(a) * s?

Actually forget my suggestion about the alternative contribution def, I don't think that is a good way to define it either because it depends heavily on the scaling of predictors.

@ErikRingen
Copy link
Copy Markdown
Contributor

View / edit / reply to this conversation on ReviewNB

ErikRingen commented on 2026-04-08T09:50:47Z ----------------------------------------------------------------

I'm concerned that, in the log-log model, \beta will be quite far from an elasticity because the input spend (x) is scaled by its max, and therefore the log1p(x) will in practice never be a good approximation of log(x).

Maybe the cleanest resolution is, for LogSaturation, not to scale the channel inputs at all.

@juanitorduz
Copy link
Copy Markdown
Collaborator Author

Thank you, @ErikRingen, for your feedback. I will take a look at it tomorrow 🙏 .

@juanitorduz juanitorduz requested a review from isofer May 5, 2026 12:47
juanitorduz and others added 2 commits May 7, 2026 22:11
Resolve conflicts after multidimensional -> mmm rename:
- Keep deprecation shim in multidimensional.py; merge branch MMM changes into mmm.py
- Merge test_multidimensional additions into test_mmm.py; remove old test file
- Fix imports (mmm_wrapper docstring, test_link, mmm_multiplicative notebook)
- Defer decomposition imports in MMMIDataWrapper to break circular import

Co-authored-by: Cursor <cursoragent@cursor.com>
@timbo112711
Copy link
Copy Markdown
Contributor

@juanitorduz this feature is awesome! 🙌🏻 I've heard a few times from folks how they can build a log or lo-log model within our framework.

Really like the link parameter functionality. Makes it really easy to choose between the three different functional forms. Erik brought up some good points in his review that I am in agreeance with.

I think the biggest concept users will need a thorough explanation is how we arrive at the different contributions under the hood.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Improvements or additions to documentation enhancement New feature or request experimental MMM priority: medium request discussion tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants