Skip to content

Fix funnel MuEffect footguns: namespaced data vars and channel-by-name API - #2907

Open
drbenvincent wants to merge 3 commits into
mainfrom
fix/funnel-mueffect-footguns-2871
Open

Fix funnel MuEffect footguns: namespaced data vars and channel-by-name API#2907
drbenvincent wants to merge 3 commits into
mainfrom
fix/funnel-mueffect-footguns-2871

Conversation

@drbenvincent

@drbenvincent drbenvincent commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Prefix DataVarMuEffect pm.Data registration as {prefix}_{var_name} so multiple effects can share dataset column names without build-time collisions
  • Add MMM.scaled_channel(channel) to select scaled spend by channel name instead of a positional index
  • Update the funnel notebook, tests, and incrementality fixtures to use the new APIs

Closes #2871

Test plan

  • pytest tests/mmm/test_additive_effect.py::TestDataVarMuEffect
  • pytest tests/mmm/test_mmm.py::TestMultidimMMMEdgeCases::test_scaled_channel_selects_by_name
  • pytest tests/mmm/test_incrementality.py::TestPostFitMutationGuard
  • pytest tests/mmm/test_incrementality.py::TestMediatedMuEffects::test_evaluator_discovers_the_effect_s_own_data
  • pytest tests/mmm/test_additive_effect.py -k "MediaMu or ControlMu"
  • Pre-commit on changed files

Made with Cursor


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

…e API

Prefix DataVarMuEffect pm.Data registration with the effect prefix and add MMM.scaled_channel so funnel notebooks can select treatment spend by name instead of positional index.

Closes #2871

Co-authored-by: Cursor <cursoragent@cursor.com>
@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 docs Improvements or additions to documentation MMM tests enhancement New feature or request priority: medium labels Aug 19, 2026
@codecov

codecov Bot commented Aug 19, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 38.46154% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.70%. Comparing base (53b2b77) to head (fd99868).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pymc_marketing/mmm/mmm.py 14.28% 6 Missing ⚠️
pymc_marketing/mmm/additive_effect.py 66.66% 2 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (53b2b77) and HEAD (fd99868). Click for more details.

HEAD has 20 uploads less than BASE
Flag BASE (53b2b77) HEAD (fd99868)
28 8
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2907       +/-   ##
===========================================
- Coverage   94.84%   71.70%   -23.14%     
===========================================
  Files         110      110               
  Lines       17389    17398        +9     
===========================================
- Hits        16492    12476     -4016     
- Misses        897     4922     +4025     

☔ View full report in Codecov by Harness.
📢 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.

@drbenvincent

Copy link
Copy Markdown
Contributor Author

Review round 1 (Bugbot + manual)

Bugbot

No bugs flagged on branch diff.

Manual review notes

Breaking change (intentional, closes #2871)

  • DataVarMuEffect now registers pm.Data as {prefix}_{var_name} instead of unprefixed dataset names.
  • Custom subclasses must read inputs via model[self.model_data_name(var)] (built-ins MediaMuEffect / ControlMuEffect updated).
  • Incrementality/counterfactual paths are compatible: graph discovery uses live pm.Data names (funnel_lf_budget in tests).

MMM.scaled_channel

  • Selects by channel_columns index (same as previous isel(channel=0) pattern, but name-safe).
  • Raises if model not built or channel missing.

Coverage added in follow-up commit

  • test_scaled_channel_selects_by_name now also asserts error when model is unbuilt.
  • DataVarMuEffect class docstring documents the namespacing contract.

Out of scope / follow-ups

Round 1 fixes pushed

  • Clarified DataVarMuEffect docstring
  • Added unbuilt-model guard test for scaled_channel

@drbenvincent drbenvincent self-assigned this Aug 19, 2026
@juanitorduz

Copy link
Copy Markdown
Collaborator

thanks @drbenvincent ! This is great! Can you please take a look into the failing notebook test :) ?

@juanitorduz juanitorduz added this to the pancake milestone Aug 19, 2026
for var_name in self.data_vars:
da = mmm.xarray_dataset[var_name]
pmd.Data(var_name, da.values, dims=da.dims)
pmd.Data(self.model_data_name(var_name), da.values, dims=da.dims)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If it is actually the same data, dont think we want differnet nodes in the graph

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 MMM priority: medium tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FunnelEffect notebook: hardcoded channel index and DataVarMuEffect name collisions

3 participants