Design for an MMMPlotSuite overall by isofer · Pull Request #2405 · pymc-labs/pymc-marketing

isofer · 2026-03-16T14:46:00Z

Summary

Design documents for the MMMPlotSuite v2 major-release rewrite. Contains planning docs that lay out the architecture, API contract, and implementation strategy for decomposing the current 5,150-line monolith into a clean namespace-based structure.

Review focus

Please focus on the first 3 sections of the main design doc (mmmplotsuite-v2-attack-plan-design.md):

Decisions
Target Architecture
Standardized API Contract

Files

2026-03-11-mmmplotsuite-v2-attack-plan-design.md -- The main design doc. Covers architectural decisions, target file layout, standardized API contract, helper removal plan, scope split (major vs follow-up), PR sequence, migration guide, and issue traceability.
2026-03-10-mmmplotsuite-comprehensive-issues.md -- Audit of the current MMMPlotSuite identifying 45 issues across architecture, API consistency, correctness, performance, and testing.
2026-03-10-mmmplotsuite-decomposition-approaches.md -- Analysis of 4 decomposition approaches (flat methods, inheritance, mixins, namespace sub-objects) with trade-offs. Concludes with the namespace sub-objects approach.
2026-03-11-figure-customization-design.md -- Deep dive into the figure customization API: how arviz-plots' PlotCollection integrates, the 6 standard customization parameters, coverage gaps for bar-plot methods, and the visuals / aes_by_visuals pattern.

juanitorduz

This looks great! I left some initial comments. IMO Variant A + D in the one I personally like the most. I would love the opinion from others like @cetagostini and @williambdean

My only important commetn: Let's not rely on ArviZ 1.0 yet. This weill need much more work and will be coupled with pymc 6 release (sometime this year I think). Hence, we need to revisit this plan to ensure it has the right ArviZ API.

I would also add that we should aim for certain level of consistency with the interactive plots module :)

juanitorduz · 2026-03-20T11:46:37Z

+The duplicated axes-normalization code (flatten + wrap in ndarray) appears in at least
+three methods, each with slightly different implementations.
+
+All of this is already handled by `arviz_plots`' `PlotCollection` API


We are still bounded by ArviZ < 1.0

juanitorduz · 2026-03-20T11:47:30Z

+
+These correspond to GitHub issues #2369–#2378 and #2388, plus additional findings.
+
+### II.1 Return type roulette (GitHub #822, #2369)


IMO we should always return (fig , axes) 🙏

Agree. That was the chosen option in the attack plan https://github.com/pymc-labs/pymc-marketing/blob/isofer/mmmplotsuite-overall/docs/plans/2026-03-11-mmmplotsuite-v2-attack-plan-design.md#return-type

juanitorduz · 2026-03-20T11:48:03Z

+
+> **Backward Compatible:** No (changing return types breaks callers; requires deprecation cycle) — **LOE:** L
+
+### II.2 Inconsistent `original_scale` default (GitHub #2371)


Uhg, I think we should always plot in original scale and get rid of this parameter once and for all.

I think there is also a value in plotting in the scaled space. So when looking at the idata or using the fundamental plotting functions of PyMC (like plotting traces) they would be in the scaled space. so I think it might be useful to plot in the space where the modeling actually happen.
I wonder what other people think about this. I'll ask in the channel.

juanitorduz · 2026-03-20T11:48:29Z

+
+
+`param_stability` is especially problematic — when `dims` is provided, it creates a
+*separate figure per dimension value* in a for-loop, calling `plt.show()` on each.


plt.show() should be removed in the codebase

agree. already in the attack plan: https://github.com/pymc-labs/pymc-marketing/blob/isofer/mmmplotsuite-overall/docs/plans/2026-03-11-mmmplotsuite-v2-attack-plan-design.md#behavioral-rules

juanitorduz · 2026-03-20T11:49:18Z

+
+These are issues found during code review that have no corresponding GitHub issue.
+
+### IV.1 Copy-paste bugs in `prior_predictive` and its helper


Uhg :/ we need to fix these docs indeed

agree. in the attack plan

juanitorduz · 2026-03-20T11:49:56Z

+### IV.7 O(n×m) inner loop in `cv_crps`
+
+`_pred_matrix_for_rows` (line 4914): Iterates over every row of `rows_df` and does
+`.sel()` per row. For large datasets this is very slow.


I think we out typical time series are rather small / medium but not really large

juanitorduz · 2026-03-20T11:50:55Z

+
+## VII. Older / Miscellaneous
+
+### VII.1 Coord name `x` → `channel` (GitHub #1183)


juanitorduz · 2026-03-20T11:56:03Z

+
+---
+
+## Comparison Matrix


Option Variant: D + A
Approach D can be combined with Approach A by backing each namespace method with a standalone function. This adds a direct function access layer (e.g., from pymc_marketing.mmm.plotting.sensitivity import sensitivity_analysis) for users who prefer working with functions and for easier unit testing. The trade-off is two public API surfaces to maintain. is the one I personally like the mostt:

isofer · 2026-03-20T16:23:53Z

My only important comment: Let's not rely on ArviZ 1.0 yet. This will need much more work and will be coupled with pymc 6 release (sometime this year I think). Hence, we need to revisit this plan to ensure it has the right ArviZ API.

@OriolAbril

@juanitorduz raised a concern about using arviz 1.0 for this plan to rewrite MMMPlotSuite.
Currently I have in my env the following:

arviz                     0.23.4             pyhcf101f3_0    conda-forge
arviz-base                1.0.0              pyhd8ed1ab_0    conda-forge
arviz-plots               1.0.0                    pypi_0    pypi
arviz-stats               1.0.0              pyh5442c79_0    conda-forge
arviz-stats-core          1.0.0              pyhc364b38_0    conda-forge

so the plots are done through arviz-plots 1.0, but I still have arviz 0.23.4 (so my idata is still az.InferenceData for example).
So I would like to have in pyproject.toml the following:

    "arviz>=0.13.0,<1.0.0",
    "arviz-plots[matplotlib]>=1.0.0,<1.1",

This seems to be working fine when I tested it. Do you have any concerns with the setup?

juanitorduz · 2026-03-20T16:25:34Z

My only important comment: Let's not rely on ArviZ 1.0 yet. This will need much more work and will be coupled with pymc 6 release (sometime this year I think). Hence, we need to revisit this plan to ensure it has the right ArviZ API.

@OriolAbril

@juanitorduz raised a concern about using arviz 1.0 for this plan to rewrite MMMPlotSuite. Currently I have in my env the following:
arviz                     0.23.4             pyhcf101f3_0    conda-forge
arviz-base                1.0.0              pyhd8ed1ab_0    conda-forge
arviz-plots               1.0.0                    pypi_0    pypi
arviz-stats               1.0.0              pyh5442c79_0    conda-forge
arviz-stats-core          1.0.0              pyhc364b38_0    conda-forge
so the plots are done through arviz-plots 1.0, but I still have arviz 0.23.4 (so my idata is still az.InferenceData for example). So I would like to have in pyproject.toml the following:
    "arviz>=0.13.0,<1.0.0",
    "arviz-plots[matplotlib]>=1.0.0,<1.1",
This seems to be working fine when I tested it. Do you have any concerns with the setup?

if we can make this work it would be great :)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…e breakdown Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…PR list Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

isofer added 8 commits March 11, 2026 09:33

adding plans

f124c15

update doc

d93640b

update plans

5bd750b

update doc

c6fcf05

add plan for CVsuite

7ad4e32

small updates

7ddfebb

update test strategy

898c719

more updates

c9f0981

juanitorduz reviewed Mar 20, 2026

View reviewed changes

minor fix

d509325

isofer and others added 12 commits March 23, 2026 17:25

updated upper half of attack plan

486fd82

update 2nd half of plan

e4a2d9d

docs: update naming conventions table — three-way var param split (II.5)

75e92e2

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

docs: update UX examples to use target_var instead of var_names

ebe9ad5

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

docs: update Quick Reference II.5 row — three-way naming convention

8d59242

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

docs: update Cross-Cutting Concerns II.5 entry

01fad04

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

docs: update PR 2 II.5 fix description to target_var

c97aeb3

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

docs: add II.5 var_name rename to PR 3 fix description

0be7a89

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

docs: add II.5 var_names rename to PR 5 fix description

5605129

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

docs: add II.5 var_names rename to PR 8 fix description

cda13b6

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

docs: expand migration guide Parameter Changes table — full var renam…

bde922a

…e breakdown Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

docs: update Issue Traceability II.5 row — three-way naming, correct …

417a7e9

…PR list Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>


		These correspond to GitHub issues #2369–#2378 and #2388, plus additional findings.

		### II.1 Return type roulette (GitHub #822, #2369)


		> Backward Compatible: No (changing return types breaks callers; requires deprecation cycle) — LOE: L

		### II.2 Inconsistent `original_scale` default (GitHub #2371)



		`param_stability` is especially problematic — when `dims` is provided, it creates a
		separate figure per dimension value in a for-loop, calling `plt.show()` on each.


		These are issues found during code review that have no corresponding GitHub issue.

		### IV.1 Copy-paste bugs in `prior_predictive` and its helper


		## VII. Older / Miscellaneous

		### VII.1 Coord name `x` → `channel` (GitHub #1183)

Conversation

isofer commented Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Review focus

Files

Uh oh!

juanitorduz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isofer commented Mar 20, 2026

Uh oh!

juanitorduz commented Mar 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

isofer commented Mar 16, 2026 •

edited

Loading

juanitorduz left a comment •

edited

Loading