MMM Plot Suite Migration: arviz_plots Multi-Backend Support #2098
Conversation
There was a problem hiding this comment.
We already added claude commands and skills to the repo so it seems appropriate to add this file.
There was a problem hiding this comment.
Can it be renamed to AGENTS.md then if this if general enough then.
There was a problem hiding this comment.
Claude.md, like the skills and commands that were already uploaded, are used by Claude code. It expect for a with that name and these set of folders.
The text in Claude.md like the skills and command is very generic and can be used by any AI Platform, but we need this specific structure to get the most of Claude code.
So I don't think it make sense to change the name of this file while leaving all the .claude folder.
The current status of the repo is weird, we uploaded skills and command which mean we support Claude Code, but we are missing Claude.md, so Claude Code cannot run on this repo.
I think we should either go all-in and support claude code and add the Claude.md file, or remove the .claude folder. I prefer to add Claude.md
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2098 +/- ##
==========================================
+ Coverage 92.57% 92.65% +0.07%
==========================================
Files 69 71 +2
Lines 9430 9760 +330
==========================================
+ Hits 8730 9043 +313
- Misses 700 717 +17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cetagostini
left a comment
cetagostini
left a comment
There was a problem hiding this comment.
Just a few docstrings changes.
There was a problem hiding this comment.
Can it be renamed to AGENTS.md then if this if general enough then.
| def test_legacy_plot_module_exists(): | ||
| """Test that legacy_plot module exists and can be imported.""" | ||
| try: | ||
| from pymc_marketing.mmm import legacy_plot | ||
|
|
||
| assert hasattr(legacy_plot, "LegacyMMMPlotSuite") | ||
| except ImportError as e: | ||
| pytest.fail(f"Failed to import legacy_plot: {e}") | ||
|
|
||
|
|
||
| def test_legacy_class_name(): | ||
| """Test that legacy suite class is named LegacyMMMPlotSuite.""" | ||
| from pymc_marketing.mmm.legacy_plot import LegacyMMMPlotSuite | ||
|
|
||
| assert LegacyMMMPlotSuite.__name__ == "LegacyMMMPlotSuite" |
There was a problem hiding this comment.
remove. These tests will be obvious failures based on other tests.
pymc_marketing/mmm/plot.py
Outdated
| def _resolve_backend(self, backend: str | None) -> str: | ||
| """Resolve backend parameter to actual backend string.""" | ||
| from pymc_marketing.mmm.config import mmm_config | ||
|
|
||
| return backend or mmm_config["plot.backend"] |
There was a problem hiding this comment.
Why not toplevel import?
There was a problem hiding this comment.
sorry, agent generated code. I didn't catch it. fixed
| def test_contributions_over_time_accepts_data_parameter(mock_posterior_data): | ||
| """Test that contributions_over_time accepts data parameter.""" | ||
| # Create suite without idata | ||
| suite = MMMPlotSuite(idata=None) | ||
|
|
||
| # Should work with explicit data parameter | ||
| pc = suite.contributions_over_time(var=["intercept"], data=mock_posterior_data) | ||
|
|
||
| assert isinstance(pc, arviz_plots.PlotCollection) | ||
|
|
||
|
|
||
| def test_contributions_over_time_data_parameter_fallback(mock_idata_with_posterior): | ||
| """Test that contributions_over_time falls back to self.idata.posterior.""" | ||
| suite = MMMPlotSuite(idata=mock_idata_with_posterior) | ||
|
|
||
| # Should work without data parameter (fallback) | ||
| pc = suite.contributions_over_time(var=["intercept"]) | ||
|
|
There was a problem hiding this comment.
@copilot use pytest.mark.parametrize to combine these two tests
There was a problem hiding this comment.
I think these are fine to keep separated.
|
@williambdean I've opened a new pull request, #2103, to work on those changes. Once the pull request is ready, I'll request review from you. |
| raise NotImplementedError( | ||
| "budget_allocation() was removed in MMMPlotSuite v2.\n\n" | ||
| "The new arviz_plots-based implementation doesn't support this chart type.\n\n" | ||
| "Alternatives:\n" | ||
| " 1. For ROI distributions: use budget_allocation_roas()\n" | ||
| " 2. To use old method: set mmm_config['plot.use_v2'] = False\n" | ||
| " 3. Implement custom bar chart using the samples data\n\n" | ||
| "See documentation: https://docs.pymc-marketing.io/en/latest/mmm/plotting_migration.html#budget-allocation" | ||
| ) |
There was a problem hiding this comment.
Is the goal to remove these missing methods entirely in 0.20.0 ? Or will they only exist for the matplotlib backend ?
CC: @cetagostini
There was a problem hiding this comment.
I've consulted with @cetagostini regarding this change, and followed his recommendation.
His point was that the current implementation of this graph is not the best, and so we decided to create budget_allocation_roas and deprecated budget_allocation entirely.
in 0.20.0 we will remove LegacyMMMPlotSuite and only the new suite will be available.
isofer
changed the title
|
@williambdean @cetagostini |
* Initial plan * Combine contributions_over_time tests using pytest.mark.parametrize Co-authored-by: williambdean <57733339+williambdean@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: williambdean <57733339+williambdean@users.noreply.github.com> Co-authored-by: Juan Orduz <juanitorduz@gmail.com>
|
bugbot run |
PR SummaryAdds a new arviz_plots-based multi-backend MMM plotting suite with a config-gated switch from the legacy matplotlib suite, plus comprehensive tests and dependency updates.
Written by Cursor Bugbot for commit e9a05ba. This will update automatically on new commits. Configure here. |
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@nialloulton Great! Cursor was able to find bug in the legacy code that we are deprecating. |
|
|
||
| - Returns PlotCollection instead of (Figure, Axes) | ||
| - Lost colors, subplot_kwargs, rc_params parameters | ||
| - Different HDI calculation (uses arviz_plots instead of custom) |
There was a problem hiding this comment.
looking at the deleted code, it looks like before az.plot_hdi was used, so the HDI calculation hasn't really changed. The only difference is internal to this method. Now hdi computation is explicit (but still implicit for users) whereas before both hdi computation and plotting was delegated to arviz in a single call.
| Breaking changes from legacy implementation: | ||
|
|
||
| - Returns PlotCollection instead of (Figure, Axes) | ||
| - Lost colors, subplot_kwargs, rc_params parameters |
There was a problem hiding this comment.
If we add a colors argument to saturation_scatterplot we can easily keep the argument here. IIUC, that argument was a list of colors with the same length as the channel dimension. As we are already adding the aes={"color": ["channel"]} in the initialization of the plotcollection in saturation_scatterplot we can add color key when initializing the plotcollection if the colors argument is provided. If not provided then we don't send anything to plotcollection and the default cycle will be used.
Example of custom aesthetic mapping cycle: https://arviz-plots.readthedocs.io/en/latest/tutorials/plots_intro.html#adding-aesthetic-mappings-to-a-visualization (first example has default cycle, second example in that section modifies it)
| Breaking changes from legacy implementation: | ||
|
|
||
| - Returns PlotCollection instead of (Figure, Axes) | ||
| - Variable names must be passed in a list (was already list in legacy) |
There was a problem hiding this comment.
this doesn't look like a breaking change, the behaviour seems unchanged according to the docstring.
Also, the previous plotting mehtod mentions different plotting saving API. I think this is a corollary of the returned object changing, but I think ideally we'd keep that consistent, either mention the bullet point everywhere or nowhere (or everywhere but as a note of the first return type bullet point)
| Breaking changes from legacy implementation: | ||
|
|
||
| - Returns PlotCollection instead of (Figure, Axes) | ||
| - Lost **kwargs for matplotlib customization (use backend-specific methods) |
There was a problem hiding this comment.
from what I can see, kwargs could only change the figure size through width_per_col and height_per_row. I would make the message either more specific or more actionable (for example pointing to how one can change the figuresize now, even if matplotlib only example).
| If additional dimensions besides 'channel', 'date', and 'sample' are present, | ||
| creates a subplot for each combination of these dimensions. | ||
| - Returns PlotCollection instead of (Figure, Axes) | ||
| - Lost scale_factor, lower_quantile, upper_quantile, figsize, ax parameters |
There was a problem hiding this comment.
if the ax parameter was there to help call the method multiple times affecting always the same figure it might be better to add a plot_collection argument to continue allowing this behaviour.
figsize could also be kept and passed to .wrap as figure_kwargs={..., "figsize": figsize}, similar to sharex. However doing this only here might not make much sense, if we want this to be easily accessible to users we should add figsize to all plots (or a way to pass arguments to .wrap or .grid methods)
| if subplot_dims[1] is not None: | ||
| title_parts.append(f"{subplot_dims[1]}={col_val}") | ||
| # plot hdi | ||
| hdi = samples[channel_contrib_var].azstats.hdi(hdi_prob, dim="sample") |
There was a problem hiding this comment.
this can be changed to azstats.eti to keep using quantile based intervals instead of switching to HDI.
MMM Plot Suite Migration: arviz_plots Multi-Backend Support
Summary
This PR introduces a new version of
MMMPlotSuitebuilt onarviz_plots, enabling multi-backend plotting (matplotlib, plotly, bokeh) while preserving full backward compatibility. The original matplotlib-only implementation is preserved asLegacyMMMPlotSuiteand remains the default in v0.18.0, with users able to opt into the new suite via configuration.This closes #2083 and #2081 (duplicated).
Key highlights:
LegacyMMMPlotSuiteKey Changes
🎨 New Multi-Backend Plotting Suite (
plot.py)Complete rewrite of
MMMPlotSuiteusing arviz_plots:PlotCollectionobjects instead of(Figure, Axes)tuplesposterior_predictive()- Posterior predictive time seriescontributions_over_time()- Variable contributions over time with HDIsaturation_scatterplot()- Channel saturation scatter plotssaturation_curves()- Saturation curves with samples and HDI overlaysbudget_allocation_roas()- Budget allocation ROAS distributionsallocated_contribution_by_channel_over_time()- Allocated contributionssensitivity_analysis()- Sensitivity analysis resultsuplift_curve()- Uplift curvesmarginal_curve()- Marginal effectsArchitectural improvements:
dataparameters with fallback toself.idata_resolve_backend()handles global config with per-method override_init_subplots(),_build_subplot_title(),_add_median_and_hdi()🔄 Backward Compatibility & Legacy Suite
Legacy Suite Created (
legacy_plot.py, 1,937 lines):LegacyMMMPlotSuiteVersion Switching (
multidimensional.py):Modified
MMM.plotproperty to support version control:Deprecation Timeline:
⚙️ Configuration System (
config.py- NEW)Added global MMM configuration (156 lines):
Features:
rcParamspatternbackendparameter on all plot methodspymc_marketing.mmmfor easy access📊 API Improvements
self.idatacontributions_over_time(data=...)saturation_scatterplot(constant_data=..., posterior_data=...)saturation_curves(constant_data=..., posterior_data=...)sensitivity_analysis(data=...),uplift_curve(data=...),marginal_curve(data=...)🧪 Comprehensive Testing (~3,100 lines of test code)
New Test Files:
tests/mmm/conftest.py(307 lines): Shared fixtures for all plotting teststests/mmm/test_plot.py(1,719 lines): Rewritten for new suitetests/mmm/test_legacy_plot.py(1,054 lines): Full legacy suite coveragetests/mmm/test_plot_compatibility.py(475 lines): Version switching testsmmm_config["plot.use_v2"]switchingtests/mmm/test_plot_data_parameters.py(120 lines): Data parameter validationtests/mmm/test_legacy_plot_imports.py(33 lines): Import validationtests/mmm/test_legacy_plot_regression.py(56 lines): Legacy behavior regression testsBreaking Changes
Return Types
Legacy (matplotlib-only):
New (multi-backend):
Parameter Changes
posterior_predictive(var=...): Now acceptsstrinstead oflist[str](call multiple times for multiple variables)ax,figsize,colors,subplot_kwargs,rc_paramsparameters (usePlotCollectionAPI instead)backendparameter for per-method overrideMethod Changes
budget_allocation(): Now a deprecated wrapper that callsbudget_allocation_roas()with deprecation warningbudget_allocation_roas(): New primary method for budget allocation visualizationsMigration Guide
Quick Start
Opting Out (Temporary)
Files Changed
Statistics: 15 files changed, 9,092 insertions(+), 1,886 deletions(-)
New Files
pymc_marketing/mmm/config.py(156 lines)pymc_marketing/mmm/legacy_plot.py(1,937 lines)tests/mmm/conftest.py(307 lines)tests/mmm/test_legacy_plot.py(1,054 lines)tests/mmm/test_legacy_plot_imports.py(33 lines)tests/mmm/test_legacy_plot_regression.py(56 lines)tests/mmm/test_plot_compatibility.py(475 lines)tests/mmm/test_plot_data_parameters.py(120 lines)CLAUDE.md(272 lines)Modified Files
pymc_marketing/mmm/__init__.py(+2 lines: exportmmm_config)pymc_marketing/mmm/plot.py(complete rewrite: ~2660 lines changed)pymc_marketing/mmm/multidimensional.py(+54 lines: version control logic)tests/mmm/test_plot.py(heavily rewritten: 1719 lines).gitignore(+3 lines)CLAUDE.md(NEW, 272 lines: project documentation)Testing
Documentation
CLAUDE.md: Added comprehensive MMM plotting architecture documentationconfig.py: Extensive docstrings with usage examplesPlotCollectionreturn typesbackendparameter documentation.. versionadded::,.. versionchanged::)Timeline
plot.use_v2 = True)Checklist
Notes
mmm_config["plot.use_v2"] = Truegit log --follow)📚 Documentation preview 📚: https://pymc-marketing--2098.org.readthedocs.build/en/2098/