plot_model_cartoon correctness: θ-reduction, seeded RNG, max_t ownership, coherent trajectories - #1131
Merged
Merged
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Contributor
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 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 |
AlexanderFengler
force-pushed
the
1122-improve-model-cartoon
branch
from
August 3, 2026 01:52
7cf31e9 to
0615b94
Compare
AlexanderFengler
force-pushed
the
1125-theta-reduction-rng
branch
from
August 3, 2026 01:52
4015e36 to
63082fb
Compare
- 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
…ng (#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
- 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
- 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
…horing (#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]
- 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
…tions (#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)
- 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
…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
force-pushed
the
1125-theta-reduction-rng
branch
from
August 3, 2026 02:14
51a26db to
2c2e60e
Compare
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.
Closes #1125. Stacked on #1130 (retarget after the chain merges).
obs=(which then conditions every simulated layer; the observed histogram stays pooled, with a warning)random_state=(int ornp.random.Generator) covers the displayed-draw selection (previously unseeded global RNG), every simulator seed, and trajectories; facets consume successive segments of one stream;rng=Nonekeeps legacy behavior for all other callersmax_tderived from xlims (was: 20 s default, ~75% of every polyline off-screen); onet_sper renderer; noisy RT sims keep the long horizon to avoid re-normalizing defective densities by censoringn_samples=1(ignoringn_reps) and one shared seed — bands summarized degenerate, noise-correlated histogramstheta_samplesbranch removed; per-trajectory boundary anchoringn_trajectories/xlims/ylimspromoted to documented params (None keeps renderer defaults)random_stateandobs=sectionsobs=coverage🤖 Generated with Claude Code