Show posterior uncertainty in plot_model_cartoon as graded bands; declutter samples mode - #1133
Merged
Merged
Conversation
Both the predictive and model-cartoon plot families need the bandable step-curve geometry and the graded-band opacity ladder. Move _curve_xy out of predictive.py and extract the inline alpha linspace into _band_alphas, so the cartoon redesign consumes the same helpers instead of copying them. No behavior change: 44/44 fast plotting tests pass unchanged.
…#1124) plot_func_model gains the #1123 uncertainty vocabulary. RT histograms are now drawn from precomputed per-draw defective-density matrices (shared bin edges, _curve_xy geometry) instead of N overlaid ax.hist calls, enabling graded pointwise quantile bands, auto-alpha spaghetti, a real step/bins story, and a hist_height normalizer. Geometry uncertainty renders as boundary fan ribbons, a dashed drift cone (decision-time aligned, NaN-masked past absorption, truncated at <50% survival), graded ndt axvspans + rug replacing the per-draw axvline smear, and a start-point whisker. uncertainty=None preserves the legacy rendering for GUI-heritage callers; add_histograms_to_twin_axes is deleted; legend is assembled from labeled artists instead of hardcoded proxies.
Public signature adopts the #1123 vocabulary: uncertainty='band' default, hdi, alpha_mean/alpha_uncertainty, hist_height, legend, live bins/step/ colors/linestyles/linewidths (previously documented but silently dead). The deprecated plot_predictive_mean/plot_predictive_samples booleans map through a FutureWarning shim; passing both spellings errors. Geometry gets its own color_model (neutral black) so the figure encodes structure=neutral, predictions=predicted color, data=observed color. The drift cone truncates at the first absorption across draws — the >=50% survival rule showed selection-biased tails. Histogram baselines anchor to the same no-noise sim that draws the reference boundary, so histograms sit exactly on the drawn bound. Hard-coded proxy legends replaced by harvested, deduped, _legend_order-ranked legends on both the 1D and FacetGrid paths.
…#1124) plot_func_model_n adopts the same machinery: per-choice defective-density matrices (jointly integrating to 1 across choices) rendered as graded bands in the per-choice trajectory colors from the single shared baseline, plus a boundary ribbon and ndt spans/rug on the geometry. uncertainty=None keeps the legacy per-draw overlays. The rewrite removes the latent NameError when only observed data was passed (bottom now always defined), anchors the baseline to the reference no-noise boundary like the 2-choice renderer, and gives the legacy legend's dashed ndt proxy its missing label.
…ng (#1124) tests/unit/plotting/test_model_cartoon.py: 30 unmarked tests in the #1123 style — pure math pins (defective-density mass identities incl. -999 exclusion and non-uniform edges, per-choice masses, geometry matrices, band-alpha ladder, shim mappings) plus artist-census assertions per mode (collection/line counts, alpha values, zorders, legend labels, live bins/step/hist_height/styles, seeded reproducibility, legacy-census parity for uncertainty=None, and the n-choice data-only NameError regression). The slow structural suite converts to the uncertainty vocabulary, keeps one boolean case covering the shim end-to-end (FutureWarning + ValueError), and adds a facet-level PolyCollection check. tests/test_model_cartoon_colors.py is kept, deviating from the original plan: _add_trajectories and its colors support survived the redesign, so the test still covers live code.
docs/tutorials/cartoon_gallery.ipynb mirrors ppc_gallery.ipynb: fixture- driven (runs in seconds, no sampling), one option per section — default graded bands, samples, both, mean-only, custom hdi, hist_height, styling, facets, the deprecated-boolean shim, and the regression-model trial- provenance caveat. Executed end-to-end: 9 figures, zero errors. Wired into mkdocs nav + execute_ignore beside its sibling. Also fixes the plotting.ipynb cell that passed long-removed kwargs (plot_pp_mean/plot_pp_samples/alpha_pp, silently swallowed since their removal) and adds the changelog entry. Full re-execution of the cartoon sections in plotting.ipynb / scientific_workflow_hssm.ipynb is deferred to merge time to avoid double churn against the #1123 notebook regeneration.
end-of-file/whitespace auto-fixes in the two gallery notebooks, and the legend dedup rewritten as an explicit loop — mypy rejects the set.add()-in-boolean-context idiom. CI's gate (mypy src/hssm) is clean; the 14 remaining local-hook findings are pre-existing in tests/addm and tests/rl on the base branch.
…#1124) Three fixes from first review of the gallery: 1. Warnings: attach_trialwise_params_to_df now sorts the (chain, draw, obs_n) MultiIndex once before its per-draw assignments — previously every assignment emitted a pandas lexsort PerformanceWarning (1236 per gallery run). The remaining numba object-mode notices trace to the fixture shipping no predictive groups (#1080); filtered in the gallery setup cell with a comment naming the cause. 2. Legend: the combined histogram + geometry legend is too tall to float inside the axes; it now sits outside on the right (center-left anchor at x=1.02). 3. Geometry coherence: all four no-noise geometry sims (reference and per-draw, both renderers) now use the trial-0 row as a single-row theta. Previously the reference simulated an unseeded RANDOM trial while the per-draw curves inherited the simulator's last-trial boundary scratch buffer — for hierarchical models the ribbons and the drawn mean bound sat at visibly different levels. One consistent trial also makes every metadata element (boundary/trajectory/ndt/z) describe the same trial and the reference deterministic. #1125 narrows to the reduction convention (trial-mean, obs=), seeded trajectories, and max_t. 4. NDT display: the graded axvspan envelope now renders in every uncertainty mode, replacing the bottom-edge rug in samples mode — an envelope hugging the dashed reference line is the natural reading; the rug read as a stray artifact. Gallery re-executed: zero warnings, 9 figures, no errors. 74/74 fast tests.
#1124) - _render_drift_band now draws fill_between quantile bands per HDI interval (widest first, _band_alphas ladder), matching the boundary ribbons and histogram bands; dashed linestyle is reserved for the non-decision-time reference line - band census test distinguishes boundary ribbons (zorder 1010) from drift bands (zorder 1012) and asserts no dashed non-vertical lines - gallery, changelog, and docstring wording updated; changelog note on trial provenance refreshed to the trial-0 convention
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Contributor
📝 WalkthroughWalkthroughThe PR adds uncertainty-aware ChangesModel cartoon uncertainty
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant plot_model_cartoon
participant plot_func_model
participant Matplotlib
User->>plot_model_cartoon: select uncertainty and styling options
plot_model_cartoon->>plot_func_model: forward resolved configuration
plot_func_model->>Matplotlib: render density bands, samples, means, and geometry
Matplotlib-->>User: display model cartoon
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hssm/plotting/model_cartoon.py (1)
158-176: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winA string
colorsis indexed per character.
colorsis typedstr | list[str] | None. Line 160 rejects a string only whenplot_data=True. Whenplot_data=Falseandcolors="red", line 169 setscolor_predictive="r"and line 171 setscolor_data="e". The style pair is then wrong.Normalize a string to a pair before building
style_kwargs.🐛 Proposed fix
colors = colors or list(DEFAULT_PREDICTIVE_COLORS) if plot_data and isinstance(colors, str): raise ValueError("When `plot_data=True`, `colors` must be a list or dict.") + if isinstance(colors, str): + colors = [colors, DEFAULT_PREDICTIVE_COLORS[1]]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hssm/plotting/model_cartoon.py` around lines 158 - 176, Normalize a string colors value to a two-entry style pair before constructing style_kwargs, so colors="red" applies the same color to predictive and data artists when plot_data=False. Update the colors handling near the existing plot_data validation, preserve list behavior and defaults, and ensure the later color_predictive and color_data indexing operates only on the normalized pair.
🧹 Nitpick comments (2)
src/hssm/plotting/model_cartoon.py (2)
2376-2414: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
plot_func_model_ndocstring is out of date.Line 2380 still documents
n_samples : int, default=10, but the parameter is nown_repsandn_samplesis only accepted through the deprecation shim at line 2416. The docstring also omits the newbins,step,intervals,hist_height,linestyle_histogram_data,linewidth_histogram_data, andalpha_uncertaintyparameters.alpha_predictiveis documented asdefault=0.05, but the signature default isNone.The sibling
plot_func_modeldocstring documents all of these. Align this one.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hssm/plotting/model_cartoon.py` around lines 2376 - 2414, Update the plot_func_model_n docstring to match its current signature and the sibling plot_func_model documentation: document n_reps instead of n_samples, add bins, step, intervals, hist_height, linestyle_histogram_data, linewidth_histogram_data, and alpha_uncertainty, and change alpha_predictive’s default to None. Keep n_samples documented only as the deprecated compatibility alias handled by the shim.
2148-2178: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
_geometry_arraysuses mapping keys as row indices.Line 2156 iterates
sims.items()and lines 2167-2176 assign into pre-allocated arrays at positioni, whereiis the dictionary key. The arrays are sizedlen(sims). The current callers buildposterior_pred_no_noisewith contiguous keys0..n-1, so this works. A caller that passes a mapping with non-contiguous or non-zero-based keys raisesIndexError, or silently leaves uninitializednp.emptyrows in the quantile computation.Index by enumeration position instead.
♻️ Proposed refactor
- for i, sample in sims.items(): + for i, sample in enumerate(sims.values()): meta = sample["metadata"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hssm/plotting/model_cartoon.py` around lines 2148 - 2178, Update _geometry_arrays to enumerate sims.items() and use the enumeration position for all assignments into b_high_m, b_low_m, drifts, ndts, and z_abs, while retaining each mapping value as sample. This must support mappings with arbitrary, non-contiguous keys without leaving uninitialized rows or raising IndexError.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/changelog.md`:
- Line 3: Update the Unreleased heading in the changelog from h3 to h2 by using
the document’s expected level, while preserving the heading text and surrounding
content.
In `@src/hssm/plotting/model_cartoon.py`:
- Around line 92-98: Update the uncertainty argument handling in the relevant
plotting helper to distinguish an omitted value from an explicit
uncertainty="band", using a sentinel default and resolving omitted values to
"band" after conflict validation. Ensure any explicitly supplied uncertainty
together with plot_predictive_mean or plot_predictive_samples raises ValueError,
preserving the documented API behavior and preventing deprecated booleans from
overriding the explicit request.
In `@tests/test_plotting_cartoon.py`:
- Around line 129-173: Restore coverage for the grouped 3-choice rendering path
in test_plot_model_cartoon_3_choice by adding at least one parametrized case
with a groups value supported by race_model_cartoon, confirming whether the
fixture expects a string or list before choosing it. Ensure the case reaches the
existing groups-not-None branch and validates the returned per-group axes; do
not remove that branch unless grouped rendering is intentionally unsupported.
In `@tests/unit/plotting/test_model_cartoon.py`:
- Around line 371-395: Update test_legacy_value_pin_plugin_mean to assert the
mean curve’s bottom anchoring using the computed bottom value instead of
normalizing it away, while retaining the shape comparison. Update
test_reproducible_with_seed to compare the collected traj_lines between seeded
renders and verify trajectory reproducibility; do not leave either local unused,
and ensure both tests pass Ruff F841.
---
Outside diff comments:
In `@src/hssm/plotting/model_cartoon.py`:
- Around line 158-176: Normalize a string colors value to a two-entry style pair
before constructing style_kwargs, so colors="red" applies the same color to
predictive and data artists when plot_data=False. Update the colors handling
near the existing plot_data validation, preserve list behavior and defaults, and
ensure the later color_predictive and color_data indexing operates only on the
normalized pair.
---
Nitpick comments:
In `@src/hssm/plotting/model_cartoon.py`:
- Around line 2376-2414: Update the plot_func_model_n docstring to match its
current signature and the sibling plot_func_model documentation: document n_reps
instead of n_samples, add bins, step, intervals, hist_height,
linestyle_histogram_data, linewidth_histogram_data, and alpha_uncertainty, and
change alpha_predictive’s default to None. Keep n_samples documented only as the
deprecated compatibility alias handled by the shim.
- Around line 2148-2178: Update _geometry_arrays to enumerate sims.items() and
use the enumeration position for all assignments into b_high_m, b_low_m, drifts,
ndts, and z_abs, while retaining each mapping value as sample. This must support
mappings with arbitrary, non-contiguous keys without leaving uninitialized rows
or raising IndexError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e7bca1e1-bc3b-431f-8333-e24877558db0
📒 Files selected for processing (10)
docs/changelog.mddocs/tutorials/cartoon_gallery.ipynbdocs/tutorials/plotting.ipynbdocs/tutorials/ppc_gallery.ipynbmkdocs.ymlsrc/hssm/plotting/model_cartoon.pysrc/hssm/plotting/predictive.pysrc/hssm/plotting/utils.pytests/test_plotting_cartoon.pytests/unit/plotting/test_model_cartoon.py
💤 Files with no reviewable changes (1)
- docs/tutorials/ppc_gallery.ipynb
👮 Files not reviewed due to content moderation or server errors (1)
- docs/tutorials/cartoon_gallery.ipynb
| @@ -1,5 +1,9 @@ | |||
| # Changelog | |||
|
|
|||
| ### Unreleased | |||
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the correct heading level for Unreleased.
### Unreleased skips the document’s expected h2 level and triggers markdownlint MD001. Change it to ## Unreleased.
Proposed fix
-### Unreleased
+## Unreleased📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ### Unreleased | |
| ## Unreleased |
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 3-3: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/changelog.md` at line 3, Update the Unreleased heading in the changelog
from h3 to h2 by using the document’s expected level, while preserving the
heading text and surrounding content.
Source: Linters/SAST tools
Comment on lines
+92
to
+98
| if plot_predictive_mean is None and plot_predictive_samples is None: | ||
| return uncertainty, alpha_mean | ||
| if uncertainty != "band": | ||
| raise ValueError( | ||
| "Pass either `uncertainty=` or the deprecated `plot_predictive_mean`/" | ||
| "`plot_predictive_samples` booleans, not both." | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Explicit uncertainty="band" plus a deprecated boolean is silently accepted.
The conflict check compares uncertainty against the default value "band". A caller who passes uncertainty="band" explicitly together with plot_predictive_mean or plot_predictive_samples does not get a ValueError. The booleans then override the explicit request. The public docstring at lines 624-629 states that this combination raises ValueError.
Use a sentinel default to detect an explicit argument, or relax the docstring wording.
♻️ Sentinel-based detection
+_UNSET = object()
+
def _resolve_uncertainty_from_legacy(
plot_predictive_mean: bool | None,
plot_predictive_samples: bool | None,
- uncertainty: Literal["band", "samples", "both"] | None,
+ uncertainty: Literal["band", "samples", "both"] | None | object,
alpha_mean: float,
) -> tuple[Literal["band", "samples", "both"] | None, float]:The public signature would then default uncertainty=_UNSET and resolve it to "band" after this call.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hssm/plotting/model_cartoon.py` around lines 92 - 98, Update the
uncertainty argument handling in the relevant plotting helper to distinguish an
omitted value from an explicit uncertainty="band", using a sentinel default and
resolving omitted values to "band" after conflict validation. Ensure any
explicitly supplied uncertainty together with plot_predictive_mean or
plot_predictive_samples raises ValueError, preserving the documented API
behavior and preventing deprecated booleans from overriding the explicit
request.
Comment on lines
129
to
+173
| [ | ||
| (2, None, False, False, "posterior_predictive", "participant_id", "stim"), | ||
| (2, None, False, False, "prior_predictive", "participant_id", "stim"), | ||
| (2, None, True, True, "posterior_predictive", "participant_id", "stim"), | ||
| (2, None, True, True, "prior_predictive", "participant_id", "stim"), | ||
| (2, None, False, True, "posterior_predictive", "participant_id", "stim"), | ||
| (2, None, False, True, "prior_predictive", "participant_id", "stim"), | ||
| (0, None, False, True, "posterior_predictive", "participant_id", "stim"), | ||
| (0, None, False, True, "prior_predictive", "participant_id", "stim"), | ||
| (0, None, True, False, "posterior_predictive", "participant_id", "stim"), | ||
| (0, None, True, False, "prior_predictive", "participant_id", "stim"), | ||
| (2, None, True, False, "posterior_predictive", "participant_id", None), | ||
| (2, None, True, False, "prior_predictive", "participant_id", None), | ||
| (2, None, True, False, "posterior_predictive", "participant_id", "stim"), | ||
| (2, None, True, False, "prior_predictive", "participant_id", "stim"), | ||
| (2, None, True, False, "posterior_predictive", None, None), | ||
| (2, None, True, False, "prior_predictive", None, None), | ||
| (2, None, "both", "posterior_predictive", "participant_id", "stim"), | ||
| (2, None, "both", "prior_predictive", "participant_id", "stim"), | ||
| (2, None, "samples", "posterior_predictive", "participant_id", "stim"), | ||
| (2, None, "samples", "prior_predictive", "participant_id", "stim"), | ||
| (0, None, "band", "posterior_predictive", "participant_id", "stim"), | ||
| (0, None, "band", "prior_predictive", "participant_id", "stim"), | ||
| (0, None, None, "posterior_predictive", "participant_id", "stim"), | ||
| (0, None, None, "prior_predictive", "participant_id", "stim"), | ||
| (2, None, None, "posterior_predictive", "participant_id", None), | ||
| (2, None, None, "prior_predictive", "participant_id", None), | ||
| (2, None, "band", "posterior_predictive", None, None), | ||
| (2, None, "band", "prior_predictive", None, None), | ||
| ], | ||
| ) | ||
| def test_plot_model_cartoon_3_choice( | ||
| race_model_cartoon, | ||
| n_trajectories, | ||
| groups, | ||
| plot_predictive_mean, | ||
| plot_predictive_samples, | ||
| uncertainty, | ||
| predictive_group, | ||
| row, | ||
| col, | ||
| ): | ||
| """Test plot_model_cartoon for 3-choice data.""" | ||
| if (not plot_predictive_mean) and (not plot_predictive_samples): | ||
| with pytest.raises(ValueError): | ||
| ax = hssm.plotting.plot_model_cartoon( | ||
| race_model_cartoon, | ||
| n_samples=10, | ||
| n_samples_prior=10, | ||
| bins=30, | ||
| col=col, | ||
| row=row, | ||
| groups=groups, | ||
| plot_predictive_mean=plot_predictive_mean, | ||
| plot_predictive_samples=plot_predictive_samples, | ||
| predictive_group=predictive_group, | ||
| alpha_mean=0.025, | ||
| n_trajectories=n_trajectories, | ||
| ) | ||
| else: | ||
| ax = hssm.plotting.plot_model_cartoon( | ||
| race_model_cartoon, | ||
| n_samples=10, | ||
| n_samples_prior=10, | ||
| bins=30, | ||
| col=col, | ||
| row=row, | ||
| groups=groups, | ||
| plot_predictive_mean=plot_predictive_mean, | ||
| plot_predictive_samples=plot_predictive_samples, | ||
| predictive_group=predictive_group, | ||
| alpha_mean=0.025, | ||
| n_trajectories=n_trajectories, | ||
| ) | ||
| ax = hssm.plotting.plot_model_cartoon( | ||
| race_model_cartoon, | ||
| n_samples=10, | ||
| n_samples_prior=10, | ||
| col=col, | ||
| row=row, | ||
| groups=groups, | ||
| uncertainty=uncertainty, | ||
| predictive_group=predictive_group, | ||
| n_trajectories=n_trajectories, | ||
| ) | ||
|
|
||
| if groups is None: | ||
| if row is not None: | ||
| assert np.all(ax.row_names == race_model_cartoon.data[row].unique()) | ||
| if col is not None: | ||
| assert np.all(ax.col_names == race_model_cartoon.data[col].unique()) | ||
| else: | ||
| assert isinstance(ax, list) | ||
| assert len(ax) == len(race_model_cartoon.data[groups].unique()) | ||
| if groups is None: | ||
| if row is not None: | ||
| assert np.all(ax.row_names == race_model_cartoon.data[row].unique()) | ||
| if col is not None: | ||
| assert np.all(ax.col_names == race_model_cartoon.data[col].unique()) | ||
| else: | ||
| assert isinstance(ax, list) | ||
| assert len(ax) == len(race_model_cartoon.data[groups].unique()) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The 3-choice grouped path lost coverage and the else branch is now dead.
Every case in the parametrization at lines 129-142 sets groups=None. The else branch at lines 171-173 therefore never runs. The 2-choice matrix still exercises the grouped path with ["dbs"] at lines 31-32.
This PR changes the grouped rendering route for multi-choice models: plot_model_cartoon forwards shared_plot_kwargs and the per-group legend flag into _plot_model_cartoon_2D at lines 925-935 of src/hssm/plotting/model_cartoon.py, and plot_func_model_n gained new geometry and legend code. No integration test now covers that combination.
Restore a grouped case, or remove the unreachable branch.
💚 Restore one grouped case
(0, None, None, "posterior_predictive", "participant_id", "stim"),
(0, None, None, "prior_predictive", "participant_id", "stim"),
+ (2, "dbs", "band", "posterior_predictive", "participant_id", "stim"),
(2, None, None, "posterior_predictive", "participant_id", None),Line 173 indexes race_model_cartoon.data[groups], so this test expects groups to be a string, unlike the 2-choice test which uses a list. Confirm which form the fixture supports before restoring the case.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| [ | |
| (2, None, False, False, "posterior_predictive", "participant_id", "stim"), | |
| (2, None, False, False, "prior_predictive", "participant_id", "stim"), | |
| (2, None, True, True, "posterior_predictive", "participant_id", "stim"), | |
| (2, None, True, True, "prior_predictive", "participant_id", "stim"), | |
| (2, None, False, True, "posterior_predictive", "participant_id", "stim"), | |
| (2, None, False, True, "prior_predictive", "participant_id", "stim"), | |
| (0, None, False, True, "posterior_predictive", "participant_id", "stim"), | |
| (0, None, False, True, "prior_predictive", "participant_id", "stim"), | |
| (0, None, True, False, "posterior_predictive", "participant_id", "stim"), | |
| (0, None, True, False, "prior_predictive", "participant_id", "stim"), | |
| (2, None, True, False, "posterior_predictive", "participant_id", None), | |
| (2, None, True, False, "prior_predictive", "participant_id", None), | |
| (2, None, True, False, "posterior_predictive", "participant_id", "stim"), | |
| (2, None, True, False, "prior_predictive", "participant_id", "stim"), | |
| (2, None, True, False, "posterior_predictive", None, None), | |
| (2, None, True, False, "prior_predictive", None, None), | |
| (2, None, "both", "posterior_predictive", "participant_id", "stim"), | |
| (2, None, "both", "prior_predictive", "participant_id", "stim"), | |
| (2, None, "samples", "posterior_predictive", "participant_id", "stim"), | |
| (2, None, "samples", "prior_predictive", "participant_id", "stim"), | |
| (0, None, "band", "posterior_predictive", "participant_id", "stim"), | |
| (0, None, "band", "prior_predictive", "participant_id", "stim"), | |
| (0, None, None, "posterior_predictive", "participant_id", "stim"), | |
| (0, None, None, "prior_predictive", "participant_id", "stim"), | |
| (2, None, None, "posterior_predictive", "participant_id", None), | |
| (2, None, None, "prior_predictive", "participant_id", None), | |
| (2, None, "band", "posterior_predictive", None, None), | |
| (2, None, "band", "prior_predictive", None, None), | |
| ], | |
| ) | |
| def test_plot_model_cartoon_3_choice( | |
| race_model_cartoon, | |
| n_trajectories, | |
| groups, | |
| plot_predictive_mean, | |
| plot_predictive_samples, | |
| uncertainty, | |
| predictive_group, | |
| row, | |
| col, | |
| ): | |
| """Test plot_model_cartoon for 3-choice data.""" | |
| if (not plot_predictive_mean) and (not plot_predictive_samples): | |
| with pytest.raises(ValueError): | |
| ax = hssm.plotting.plot_model_cartoon( | |
| race_model_cartoon, | |
| n_samples=10, | |
| n_samples_prior=10, | |
| bins=30, | |
| col=col, | |
| row=row, | |
| groups=groups, | |
| plot_predictive_mean=plot_predictive_mean, | |
| plot_predictive_samples=plot_predictive_samples, | |
| predictive_group=predictive_group, | |
| alpha_mean=0.025, | |
| n_trajectories=n_trajectories, | |
| ) | |
| else: | |
| ax = hssm.plotting.plot_model_cartoon( | |
| race_model_cartoon, | |
| n_samples=10, | |
| n_samples_prior=10, | |
| bins=30, | |
| col=col, | |
| row=row, | |
| groups=groups, | |
| plot_predictive_mean=plot_predictive_mean, | |
| plot_predictive_samples=plot_predictive_samples, | |
| predictive_group=predictive_group, | |
| alpha_mean=0.025, | |
| n_trajectories=n_trajectories, | |
| ) | |
| ax = hssm.plotting.plot_model_cartoon( | |
| race_model_cartoon, | |
| n_samples=10, | |
| n_samples_prior=10, | |
| col=col, | |
| row=row, | |
| groups=groups, | |
| uncertainty=uncertainty, | |
| predictive_group=predictive_group, | |
| n_trajectories=n_trajectories, | |
| ) | |
| if groups is None: | |
| if row is not None: | |
| assert np.all(ax.row_names == race_model_cartoon.data[row].unique()) | |
| if col is not None: | |
| assert np.all(ax.col_names == race_model_cartoon.data[col].unique()) | |
| else: | |
| assert isinstance(ax, list) | |
| assert len(ax) == len(race_model_cartoon.data[groups].unique()) | |
| if groups is None: | |
| if row is not None: | |
| assert np.all(ax.row_names == race_model_cartoon.data[row].unique()) | |
| if col is not None: | |
| assert np.all(ax.col_names == race_model_cartoon.data[col].unique()) | |
| else: | |
| assert isinstance(ax, list) | |
| assert len(ax) == len(race_model_cartoon.data[groups].unique()) | |
| [ | |
| (2, None, "both", "posterior_predictive", "participant_id", "stim"), | |
| (2, None, "both", "prior_predictive", "participant_id", "stim"), | |
| (2, None, "samples", "posterior_predictive", "participant_id", "stim"), | |
| (2, None, "samples", "prior_predictive", "participant_id", "stim"), | |
| (0, None, "band", "posterior_predictive", "participant_id", "stim"), | |
| (0, None, "band", "prior_predictive", "participant_id", "stim"), | |
| (0, None, None, "posterior_predictive", "participant_id", "stim"), | |
| (0, None, None, "prior_predictive", "participant_id", "stim"), | |
| (2, "dbs", "band", "posterior_predictive", "participant_id", "stim"), | |
| (2, None, None, "posterior_predictive", "participant_id", None), | |
| (2, None, None, "prior_predictive", "participant_id", None), | |
| (2, None, "band", "posterior_predictive", None, None), | |
| (2, None, "band", "prior_predictive", None, None), | |
| ], | |
| ) | |
| def test_plot_model_cartoon_3_choice( | |
| race_model_cartoon, | |
| n_trajectories, | |
| groups, | |
| uncertainty, | |
| predictive_group, | |
| row, | |
| col, | |
| ): | |
| """Test plot_model_cartoon for 3-choice data.""" | |
| ax = hssm.plotting.plot_model_cartoon( | |
| race_model_cartoon, | |
| n_samples=10, | |
| n_samples_prior=10, | |
| col=col, | |
| row=row, | |
| groups=groups, | |
| uncertainty=uncertainty, | |
| predictive_group=predictive_group, | |
| n_trajectories=n_trajectories, | |
| ) | |
| if groups is None: | |
| if row is not None: | |
| assert np.all(ax.row_names == race_model_cartoon.data[row].unique()) | |
| if col is not None: | |
| assert np.all(ax.col_names == race_model_cartoon.data[col].unique()) | |
| else: | |
| assert isinstance(ax, list) | |
| assert len(ax) == len(race_model_cartoon.data[groups].unique()) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_plotting_cartoon.py` around lines 129 - 173, Restore coverage for
the grouped 3-choice rendering path in test_plot_model_cartoon_3_choice by
adding at least one parametrized case with a groups value supported by
race_model_cartoon, confirming whether the fixture expects a string or list
before choosing it. Ensure the case reaches the existing groups-not-None branch
and validates the returned per-group axes; do not remove that branch unless
grouped rendering is intentionally unsupported.
Comment on lines
+371
to
+395
| def test_legacy_value_pin_plugin_mean(self): | ||
| """The None-mode mean curve equals the independently computed plug-in.""" | ||
| from ssms.basic_simulators.simulator import simulator | ||
|
|
||
| theta_mean, _ = _theta_frames() | ||
| np.random.seed(0) | ||
| rand_int = np.random.randint(0, 400000000) | ||
| sim_out = simulator( | ||
| model="ddm", | ||
| theta=theta_mean.values, | ||
| n_samples=2, | ||
| no_noise=False, | ||
| delta_t=0.01, | ||
| random_state=rand_int, | ||
| ) | ||
| edges = np.arange(-0.05, 5, 0.05) | ||
| expected_up, _ = _defective_densities(sim_out["rts"], sim_out["choices"], edges) | ||
| fig, ax, up, down = _render(None, data=None) | ||
| x, y = _curve_xy(edges, expected_up, True) | ||
| mean_line = up.get_lines()[0] | ||
| bottom = mean_line.get_ydata().min() if expected_up.min() == 0 else None | ||
| np.testing.assert_allclose( | ||
| mean_line.get_ydata() - (mean_line.get_ydata() - y).min(), y, atol=1e-8 | ||
| ) | ||
| plt.close("all") |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Two tests contain unused locals that mark missing assertions.
Line 391 assigns bottom and never uses it. Line 473 assigns traj_lines and never uses it. Ruff reports F841 for both, and the coding guidelines require Ruff compliance.
Each unused local also marks an assertion that is absent:
test_legacy_value_pin_plugin_meancomputes the expected bottom offset but then normalizes it away at line 393 with- (mean_line.get_ydata() - y).min(). The test therefore verifies curve shape but not the bound anchoring, which is the behavior lines 1215-1225 ofsrc/hssm/plotting/model_cartoon.pychanged.test_reproducible_with_seedpassesn_trajectories=2and collects the trajectory lines, but line 474 compares onlyax.get_lines()[:2]. Trajectory reproducibility is not asserted, althoughplot_func_modelseeds trajectory simulation withrand_int + iat line 1211.
Add the missing assertions, or remove the unused locals.
💚 Assert trajectory reproducibility
def test_reproducible_with_seed(self):
results = []
for _ in range(2):
fig, ax, up, down = _render("band", n_trajectories=2)
- traj_lines = [ln for ln in ax.get_lines() if 2000 <= ln.get_zorder() < 3000]
- results.append([ln.get_ydata().copy() for ln in ax.get_lines()[:2]])
+ results.append([ln.get_ydata().copy() for ln in ax.get_lines()])
plt.close("all")
+ assert len(results[0]) == len(results[1])
for a, b in zip(*results):
np.testing.assert_array_equal(a, b)As per coding guidelines: "Use Ruff for linting and formatting, and comply with the configured Ruff rules."
Also applies to: 469-477
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/plotting/test_model_cartoon.py` around lines 371 - 395, Update
test_legacy_value_pin_plugin_mean to assert the mean curve’s bottom anchoring
using the computed bottom value instead of normalizing it away, while retaining
the shape comparison. Update test_reproducible_with_seed to compare the
collected traj_lines between seeded renders and verify trajectory
reproducibility; do not leave either local unused, and ensure both tests pass
Ruff F841.
Source: Coding guidelines
AlexanderFengler
added a commit
that referenced
this pull request
Aug 3, 2026
…cal (#1125) CodeRabbit findings on #1133, applied here at the stack tip to avoid a rebase round: the 3-choice parametrization never sets groups (the race fixture has no grouping column; the renderer-agnostic grouped path is covered by the 2-choice ['dbs'] rows), and the value-pin test carried an unused 'bottom' local.
Member
Author
|
CodeRabbit adjudication (all four findings are minor; none block this PR):
|
AlexanderFengler
added a commit
that referenced
this pull request
Aug 3, 2026
…cal (#1125) CodeRabbit findings on #1133, applied here at the stack tip to avoid a rebase round: the 3-choice parametrization never sets groups (the race fixture has no grouping column; the renderer-agnostic grouped path is covered by the 2-choice ['dbs'] rows), and the value-pin test carried an unused 'bottom' local.
AlexanderFengler
added a commit
that referenced
this pull request
Aug 3, 2026
…hip, coherent trajectories (#1131) * refactor: single t_s/max_t ownership derived from xlims (#1125) - one geometry horizon per renderer: max_t_geom = xlim_high + 0.5, passed to every no-noise geometry and trajectory simulation (was: simulator's 20 s default, ~75% of every polyline outside the axes) - t_s computed once per renderer; the four scattered metadata-derived assignments removed (also fixes the _add_trajectories branch leak) - noisy RT sims deliberately keep the long default horizon: shortening it would censor slow RTs into -999 and re-normalize defective densities - _geometry_arrays hardened: constant-extends short boundary arrays and no longer assumes dict keys are 0..n-1 * feat: _reduce_theta — trial-mean geometry by default, obs= conditioning (#1125) - new _reduce_theta(theta, obs): one θ vector per (chain, draw) — the across-trials mean by default, or the obs_n-labeled trial's row — so boundary, drift, ndt, and starting point describe the same coherent θ by construction (replaces the interim trial-0 convention) - RT histograms stay marginal over trials by default (they are a check against pooled observed data); obs= conditions every simulated layer on that trial, with a warning that the observed histogram stays pooled - theta_mean keeps its obs_n labels (droplevel instead of reset_index); trajectory trial picks switched to positional iloc so facet-local, non-contiguous labels cannot mis-select - new obs parameter on plot_model_cartoon and both renderers, validated at the public level and label-checked per facet inside the reduction * feat: seeded RNG end-to-end via np.random.default_rng (#1125) - plot_model_cartoon gains random_state (int or Generator); one Generator now drives which posterior draws are displayed (previously selected by the unseeded global RNG before the renderer-level seed ever applied), every simulator seed, and the trajectories - rng= threaded through _use... draw selection helpers (_get_plotting_df/_xarray_to_df/_random_sample/_generate_random_indices) with None defaults, so plot_predictive and other callers are untouched - renderers replace the np.random.seed + randint protocol with positional draws from the Generator, eliminating the rand_int + i seed collision between per-draw sims and trajectories (and, in the n-choice renderer, the constant seed shared by all draws) - facets consume successive segments of one stream: distinct per facet, reproducible across calls; stream order documented on random_state - tests: value pin re-pinned to the new protocol; reproducibility test now asserts trajectories, bands, and ndt spans (it previously computed trajectory lines and never asserted them); Generator statefulness and seeded _generate_random_indices covered * fix: n-choice per-draw sims respect n_reps (#1125) - plot_func_model_n passed n_samples=1 to every per-draw RT simulation, silently ignoring n_reps: the per-choice uncertainty bands summarized single-rep, near-degenerate histograms (the companion constant-seed bug fell with the Generator protocol in the previous commit) - recording-wrapper test pins n_reps and pairwise-distinct seeds * fix: trajectories realize the reduced reference θ; per-trajectory anchoring (#1125) - trajectories are noisy realizations of the SAME reduced θ that draws the reference geometry, so they illustrate diffusion noise around the bounds actually on screen and their crossing markers land on the drawn boundary by construction (was: each trajectory simulated a different random trial's θ but was rendered against the reference geometry) - the dead theta_samples trajectory branch is removed: unreachable from the public entry point (run_mean is always True) and incoherent — it composed θ from independent chain/draw/obs picks; direct renderer calls without theta_mean now use one real draw's reduced θ - _add_trajectories/_add_trajectories_n derive bounds and ndt-roll per trajectory from its own metadata instead of sample[0] * feat: seeded draw subsampling in _use_traces_or_sample (#1125) - when predictive sampling is triggered with an rng and an integer n_samples, pass a sorted seeded random subset of posterior draw indices to sample_posterior_predictive instead of the legacy first-n (deterministic but biased toward early, warm-up-adjacent draws) - rng=None (every non-cartoon caller) keeps the legacy behavior exactly * docs+api: promote n_trajectories/xlims/ylims; changelog + gallery sections (#1125) - n_trajectories/xlims/ylims promoted from undocumented **kwargs to documented parameters with None sentinels (renderer defaults kept); xlims is now load-bearing — it determines the geometry max_t horizon - named parameters win over the legacy kwargs spellings (popped, no duplicate-keyword errors) - changelog: #1125 entry with the deliberate value changes; the #1124 entry's trial-provenance note now points at it - cartoon_gallery: new 'Reproducible figures with random_state' and 'Conditioning on one trial with obs=' sections; caveat section rewritten for the trial-mean convention; re-executed (11 figures, zero warnings) * test: slow-suite random_state/obs coverage (#1125) - random_state=42 on every 2-/3-choice parametrization row (end-to-end threading incl. facets and groups) - double-render equality test: same seed => identical figure through the full public path (after a warm-up render materializes the predictive group so both snapshots take the identical code path) - obs=0 conditioning on the cavanagh regression model; out-of-range obs raises the labeled ValueError * test: drop dead grouped-else in the 3-choice suite and a vestigial local (#1125) CodeRabbit findings on #1133, applied here at the stack tip to avoid a rebase round: the 3-choice parametrization never sets groups (the race fixture has no grouping column; the renderer-agnostic grouped path is covered by the 2-choice ['dbs'] rows), and the value-pin test carried an unused 'bottom' local. * docs: re-execute plotting and scientific-workflow tutorials (#1125) Deferred from #1133 to avoid double churn: the cartoon cells now render with the final #1124 + #1125 behavior (graded bands, trial-mean geometry, reference-θ trajectories). Both notebooks are execute_ignore in mkdocs, so these committed outputs are the rendered docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #1130, which was auto-closed when the #1123 base branch was deleted and cannot be reopened after the rebase onto main (same commits, content unchanged — rebased past the #1123 squash). All review context lives on #1130.
Closes #1124. Stacked on #1123 (reuses its helpers and
uncertainty=vocabulary); retarget tomainwhen #1123 merges.What changed
uncertainty="band" | "samples" | "both" | Noneonplot_model_cartoon, default"band"— same vocabulary asplot_predictive.Nonereproduces the old mean-only look (test-pinned).ax.histcalls (~0.98 accumulated opacity at defaults). Makes the previously deadbins/step/colors/linestyles/linewidthsparameters real; addshist_height.axvspans + rug replacing the per-draw axvline smear, starting-point whisker. Geometry stays neutral black (color_model); predictions/data keep the predictive palette.samplesmode: auto per-curve alphaclip(4/n_draws, .02, .25)instead of fixed 0.05.plot_predictive_mean/plot_predictive_samples→FutureWarning; both spellings together error.docs/tutorials/cartoon_gallery.ipynb(sibling ofppc_gallery.ipynb, executed, 9 figures). Changelog entry included.tests/unit/plotting/test_model_cartoon.py); slow suite converted to the new vocabulary + one shim-coverage case.Scope notes
plotting.ipynb/scientific_workflow_hssm.ipynb: stale removed-kwargs fixed in source; full re-execution deferred to merge time to avoid double churn against Show posterior uncertainty in plot_predictive by default; add samples and KDE displays #1123's notebook regeneration.Commands run
uv run pytest tests/unit/plotting/→ 74/74uv run mypy src/hssm(CI's gate) → clean;uv run ruff check .→ clean except a pre-existing unused import inppc_gallery.ipynbon the base branch🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Bug Fixes