Skip to content

Incrementality - the rest of the story#2300

Open
isofer wants to merge 86 commits intomainfrom
isofer/roas-the-rest
Open

Incrementality - the rest of the story#2300
isofer wants to merge 86 commits intomainfrom
isofer/roas-the-rest

Conversation

@isofer
Copy link
Contributor

@isofer isofer commented Feb 16, 2026

Description

This PR addresses three areas:

  1. Input validation when users pass filtered or aggregated idata to Incrementality.
  2. Wiring incremental ROAS as an opt-in method through the summary and plot_interactive layers (default remains "elementwise" for backward compatibility).
  3. API cleanup (period_start/period_endstart_date/end_date, get_roasget_elementwise_roas).

Problem 1: No validation for filtered/aggregated idata in Incrementality

Users could pass pre-aggregated (e.g. aggregate_dims) or improperly filtered (scalar filter_dims) idata to Incrementality, resulting in incorrect results or cryptic errors. The adstock model graph expects the original per-dimension-value parameters, so these transformations are incompatible with counterfactual evaluation.

Approach:

  1. MMMIDataWrapper.compare_coords — compares coordinate values between the wrapper's idata and the fitted PyMC model for each custom dimension (excluding date), returning (in_model_not_idata, in_idata_not_model) dicts.

  2. Incrementality.__init__ validation — on construction, calls compare_coords and raises ValueError if the idata contains unknown coordinate labels (aggregated dims) or is missing a dimension entirely (scalar filter that dropped the dim). Provides actionable guidance in the error message.

Problem 2: summary.roas() and plot_interactive.roas() only support naive element-wise ROAS

summary.roas() and plot_interactive.roas() used simple contribution/spend division, which does not account for adstock carryover. This gives misleading ROAS values, especially for channels with long carryover windows. Users now have the option to compute true incremental ROAS via method="incremental".

Approach:

  1. data.get_roasdata.get_elementwise_roas rename — the old get_roas() is deprecated with a FutureWarning pointing users to get_elementwise_roas() or summary.roas(method="incremental").

  2. summary.roas(method=...) parameter — adds method ("elementwise" default, "incremental" opt-in), include_carryover, num_samples, random_state, start_date, and end_date parameters. When method="incremental", delegates to Incrementality.contribution_over_spend; when "elementwise", uses the data wrapper directly. Validates that incremental-only parameters aren't used with method="elementwise".

  3. plot_interactive.roas(method=...) parameter — passes the same parameters through to summary.roas().

Refactoring & API cleanup

  1. period_start/period_endstart_date/end_date — renamed across the Incrementality API (public methods, internal helpers, docstrings, and the example in multidimensional.py). MMMSummaryFactory.roas() and MMMPlotlyFactory.roas() received start_date/end_date as new parameters (they had no date parameters before).

  2. Incrementality.__init__ now accepts data parameter — accepts either idata or data (mutually exclusive). Passing a pre-built MMMIDataWrapper avoids redundant re-wrapping and enables passing data directly from summary.roas().

  3. conftest.py fixmock_fit now calls _set_xarray_data so channel_data has concrete shape matching real mmm.fit() behavior.

Related Issue

Examples

Element-wise ROAS (default)

Incremental ROAS (opt-in)

# Incremental ROAS (requires model)
df = mmm.summary.roas(method="incremental", frequency="quarterly")

# With subsampling for speed
df = mmm.summary.roas(
    method="incremental", frequency="monthly", num_samples=100, random_state=42
)

# Via plot_interactive
fig = mmm.plot_interactive.roas(method="incremental", frequency="all_time")

Known Limitations

method="incremental" does not support filter_dims or aggregate_dims on the data
passed to roas(). The Incrementality class validates that idata coordinates match
the fitted model exactly, so pre-filtered or pre-aggregated data will raise ValueError.

@juanitorduz juanitorduz added this to the 1.0 milestone Feb 24, 2026
Base automatically changed from work-issue-2211 to main February 24, 2026 20:01
@isofer isofer requested a review from ricardoV94 February 24, 2026 22:07
@codecov
Copy link

codecov bot commented Feb 24, 2026

Codecov Report

❌ Patch coverage is 87.32394% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.15%. Comparing base (fbc2831) to head (291a26f).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
pymc_marketing/data/idata/mmm_wrapper.py 84.61% 4 Missing ⚠️
pymc_marketing/mmm/incrementality.py 88.00% 3 Missing ⚠️
pymc_marketing/mmm/summary.py 88.88% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2300      +/-   ##
==========================================
- Coverage   93.18%   93.15%   -0.03%     
==========================================
  Files          79       79              
  Lines       12472    12526      +54     
==========================================
+ Hits        11622    11669      +47     
- Misses        850      857       +7     

☔ 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.

- Remove _EvaluatorContext and _build_evaluator (shared variable swapping)
- Restore inline evaluator compilation in compute_incremental_contribution
- Simplify _validate_input to date-only validation
- Add compare_coords guard at __init__ time

Made-with: Cursor
…nd plot_interactive.roas()

Made-with: Cursor
Copy link

@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.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

@isofer isofer changed the title Incrementality - the rest of the story WIP: Incrementality - the rest of the story Feb 25, 2026
@isofer isofer marked this pull request as draft February 25, 2026 21:53
@isofer
Copy link
Contributor Author

isofer commented Feb 26, 2026

@cursoragent review

@pymc-labs pymc-labs deleted a comment from cursor bot Feb 26, 2026
@isofer isofer marked this pull request as ready for review February 26, 2026 19:16
@isofer isofer changed the title WIP: Incrementality - the rest of the story Incrementality - the rest of the story Feb 26, 2026
@isofer
Copy link
Contributor Author

isofer commented Feb 26, 2026

@juanitorduz @ricardoV94 this is ready to review

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 MMM Needs Triage tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants