Wire the simulation modes into the playground, on qMRLab's conventions - #31
Merged
Conversation
added 30 commits
August 2, 2026 22:23
Move sim.trials/snr validation into every run_* entry point (not just the CLI), so a browser panic can no longer trap the wasm module; re-derive Fit armed state and the protocol lock on a Simulate mode switch instead of leaving both stuck; fix the montecarlo axis label and single-voxel note text; print a model's own parameter symbols verbatim instead of title-casing them; and drop app.simReport/app.simMode, which nothing outside sim.js reads. Claude-Session: https://claude.ai/code/session_01DSdowSspKCv3nE5sN2WirM
A diverged fit crosses the JSON boundary as null, which coerced to 0 in arithmetic and got drawn as real error/scatter data; sensitivity and histogram reference lines could render outside their own axis range; the histogram's two markLines were indistinguishable; a degenerate histogram bar and narrower-than-bin bars misrepresented the distribution.
…floating surface Rewire the data/sim toggle as a checkbox-driven switch instead of two buttons, move it to the front of the navbar, and give modals, the ROI toolbox, and the segmentation menu the tooltip's glass treatment.
added 4 commits
August 4, 2026 00:36
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change makes simulation recipes mandatory, centralizes noise-kind metadata and validation, expands simulation reports, and adds a worker-backed browser simulation workflow with charts, cancellation, loading states, tests, and documentation. ChangesSimulation infrastructure and playground
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@crates/qmrust-core/src/config.rs`:
- Around line 151-159: Update the enabled-noise SNR validation in the
configuration validation flow after NoiseKind parsing to require self.noise.snr
to be finite and greater than zero, rejecting NaN and positive infinity while
preserving the NoiseKind::None behavior.
In `@crates/qmrust-core/src/sim/mod.rs`:
- Around line 196-197: Update the fitted_signal construction in the simulation
flow to start from the configured truth parameter vector and merge the reported
fitted values into it, rather than using fitted_to_param_vec, so non-reported
parameters retain their configured values. Keep the subsequent model.forward and
measurement_values calls unchanged.
In `@docs/playground/app.css`:
- Around line 1198-1201: Update the `#status`::before declaration by replacing
each currentColor keyword in background and box-shadow with the
Stylelint-required currentcolor spelling, without changing the styling behavior.
In `@docs/playground/sim-series.js`:
- Around line 156-161: Update errorHistogram to compute the finite values’
minimum and maximum in a single pass instead of spreading finite into Math.min
and Math.max, preserving the existing empty and constant-value results. Apply
the same change to sharedRange in sim.js, replacing both spread-based extent
calculations with a safe single-pass approach.
In `@docs/playground/sim.js`:
- Around line 304-323: Update sharedRange to detect an empty values list before
calling Math.min or Math.max, including the param.points.flat() case in
montecarloOption. Return a finite fallback range for empty input so panel axes
and diagonal markLine coordinates remain valid, while preserving the existing
range calculation for non-empty values.
- Around line 492-506: Update the xRanges calculation to handle a degenerate x
extent where lo === hi, using the same absolute fallback padding pattern already
applied in yRanges. Preserve proportional padding for non-flat ranges and ensure
single-point sweeps receive a nonzero axis range.
In `@docs/playground/skeleton.js`:
- Around line 89-92: The cycleMs function in docs/playground/skeleton.js (89-92)
must clamp the computed duration to at least one step, including for 1x1 grids;
preserve the existing scaled traversal behavior for larger grids. In
scripts/tests/skeleton.test.mjs (68-84), update the cycleMs(cols, rows) test to
assert the returned duration is greater than zero before comparing firstClearsAt
with lastLitAt.
In `@scripts/check_source_hygiene.mjs`:
- Around line 241-285: The flattenAtRules flow in
scripts/check_source_hygiene.mjs must retain an effective condition key for each
display selector and require matching [hidden] overrides to share that condition
scope, rather than treating conditional rules as unconditional. Add the
requested regression case in scripts/tests/source_hygiene.test.mjs lines 32-52:
an unconditional display rule with its hidden override only inside an unmatched
media or supports condition, and assert hiddenDisplayGaps reports the gap.
🪄 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: dbaab639-098f-4f67-8765-a3a6f630bd49
📒 Files selected for processing (50)
crates/qmrust-cli/src/catalog.rscrates/qmrust-core/src/config.rscrates/qmrust-core/src/core/model.rscrates/qmrust-core/src/registry.rscrates/qmrust-core/src/sim/mod.rscrates/qmrust-core/src/sim/noise.rscrates/qmrust-core/src/sim/report.rscrates/qmrust-core/tests/properties.rsdocs/agents/ARCHITECTURE.mddocs/guide/simulation.mddocs/playground.mddocs/playground/app.cssdocs/playground/app.jsdocs/playground/data/b1_afi.jsondocs/playground/data/b1_dam.jsondocs/playground/data/index.jsondocs/playground/data/inversion_recovery.jsondocs/playground/data/mono_t2.jsondocs/playground/data/mt_ratio.jsondocs/playground/data/mt_sat.jsondocs/playground/data/qmt_spgr.jsondocs/playground/data/vfa_t1.jsondocs/playground/dataset.jsdocs/playground/dom.jsdocs/playground/fit.jsdocs/playground/index.htmldocs/playground/inputs.jsdocs/playground/labels.jsdocs/playground/model.jsdocs/playground/recipe.jsdocs/playground/sim-series.jsdocs/playground/sim-worker.jsdocs/playground/sim.jsdocs/playground/skeleton.jsdocs/playground/state.jsdocs/playground/vendor/icons.jsrecipes/sim/b1_afi_sim.yamlrecipes/sim/b1_dam_sim.yamlrecipes/sim/irt1_sim.yamlrecipes/sim/mono_t2_sim.yamlrecipes/sim/mt_ratio_sim.yamlrecipes/sim/mt_sat_sim.yamlrecipes/sim/qmt_sim_ramani.yamlrecipes/sim/vfa_t1_sim.yamlscripts/check_source_hygiene.mjsscripts/make_docs_figures.pyscripts/tests/sim_series.test.mjsscripts/tests/skeleton.test.mjsscripts/tests/source_hygiene.test.mjsscripts/tests/test_dataset.py
Comment on lines
+1198
to
+1201
| #status::before { | ||
| content: ""; display: inline-block; width: 7px; height: 7px; margin-right: 6px; | ||
| border-radius: 50%; background: currentColor; | ||
| box-shadow: 0 0 5px currentColor, 0 0 2px currentColor; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the keyword spelling required by Stylelint.
Line 1200 and Line 1201 use currentColor. The configured Stylelint rule requires currentcolor, so the style check fails.
Proposed fix
- border-radius: 50%; background: currentColor;
- box-shadow: 0 0 5px currentColor, 0 0 2px currentColor;
+ border-radius: 50%; background: currentcolor;
+ box-shadow: 0 0 5px currentcolor, 0 0 2px currentcolor;📝 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
| #status::before { | |
| content: ""; display: inline-block; width: 7px; height: 7px; margin-right: 6px; | |
| border-radius: 50%; background: currentColor; | |
| box-shadow: 0 0 5px currentColor, 0 0 2px currentColor; | |
| `#status`::before { | |
| content: ""; display: inline-block; width: 7px; height: 7px; margin-right: 6px; | |
| border-radius: 50%; background: currentcolor; | |
| box-shadow: 0 0 5px currentcolor, 0 0 2px currentcolor; |
🧰 Tools
🪛 Stylelint (17.14.1)
[error] 1200-1200: Expected "currentColor" to be "currentcolor" (value-keyword-case)
(value-keyword-case)
[error] 1201-1201: Expected "currentColor" to be "currentcolor" (value-keyword-case)
(value-keyword-case)
[error] 1201-1201: Expected "currentColor" to be "currentcolor" (value-keyword-case)
(value-keyword-case)
🤖 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/playground/app.css` around lines 1198 - 1201, Update the `#status`::before
declaration by replacing each currentColor keyword in background and box-shadow
with the Stylelint-required currentcolor spelling, without changing the styling
behavior.
Source: Linters/SAST tools
added 10 commits
August 4, 2026 01:22
… non-finite SNR Claude-Session: https://claude.ai/code/session_01DSdowSspKCv3nE5sN2WirM
… the ground truth Claude-Session: https://claude.ai/code/session_01DSdowSspKCv3nE5sN2WirM
…und an umbrella void Claude-Session: https://claude.ai/code/session_01DSdowSspKCv3nE5sN2WirM
… the fit runs Claude-Session: https://claude.ai/code/session_01DSdowSspKCv3nE5sN2WirM
agahkarakuzu
force-pushed
the
feat/playground-simulations
branch
from
August 4, 2026 21:50
7a945c7 to
4a26a10
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
scripts/make_docs_figures.py (1)
487-490: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMerge the recipe comment so the count is stated once.
The comment above at line 478 still opens with "Two recipes, because a recipe is protocol + options and where the protocol comes from differs by input." The new comment at line 487 opens with "Three recipes" and repeats the same clause. The two comments now contradict each other on the count and duplicate the explanation. State the count once and describe all three recipes in one block.
As per coding guidelines: "Remove dead code, commented-out code, speculative scaffolding, obsolete compatibility layers, duplicated sources of truth, and stale terminology."
♻️ Proposed change
- # Two recipes, because a recipe is protocol + options and where the - # protocol comes from differs by input. The pre-baked slice has no + # Three recipes, because a recipe is protocol + options and where the + # protocol comes from differs by input. The pre-baked slice has no # sidecars, so its recipe carries the acquisition (`non_bids`). A fetched # BIDS dataset resolves its own acquisition from its sidecars, so its # recipe carries options only (`bids`) and `resolve_bids` supplies the - # protocol. Both paths come from the registry's declared recipe paths — - # never a filename guessed here. + # protocol. The sim recipe carries the acquisition plus a sim: block of + # ground-truth parameters, and reads no image data. All three paths come + # from the registry's declared recipe paths — never a filename guessed + # here. "config": (repo_root / model["recipes"]["non_bids"]).read_text(), "config_bids": (repo_root / model["recipes"]["bids"]).read_text(), - # Three recipes, because a recipe is protocol + options and where the - # protocol comes from differs: the sim recipe carries the acquisition - # and a sim: block of ground-truth parameters, and reads no image data. "config_sim": (repo_root / model["recipes"]["sim"]).read_text(),🤖 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 `@scripts/make_docs_figures.py` around lines 487 - 490, Consolidate the adjacent recipe comments into one block with a single, correct statement that there are three recipes and a unified explanation of their protocol and input differences. Remove the duplicated or contradictory wording while preserving the existing recipe-specific details near the config_sim definition.Source: Coding guidelines
docs/playground/sim.js (2)
155-177: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDiscard the worker after a failed run.
A failed run can come from a wasm panic. A panicked wasm instance stays unusable, and
ensureWorkerreuses the cached worker for every later run, so all later simulations would fail until a page reload. Terminate the worker on the failure path so the next run creates a fresh instance.♻️ Proposed change
if (!report.ok) { + // A failure can be a wasm panic, which leaves the instance unusable, so + // the next run starts from a fresh worker. + worker?.terminate(); + worker = null; status("Simulation failed", "error"); showNotice("triangle-alert", "Simulation failed", report.error); return; }🤖 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/playground/sim.js` around lines 155 - 177, In the failed-run branch of the simulation flow, terminate and discard the cached worker before displaying the failure notice, so the next ensureWorker call creates a fresh instance. Update the worker reference/state used by ensureWorker alongside the termination, while preserving the existing success and cancellation paths.
739-754: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd arrow-key navigation between simulation tabs. Support
ArrowLeftandArrowRightaccording to the ARIA tab pattern.🤖 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/playground/sim.js` around lines 739 - 754, Update buildModeTabs to add ARIA tab arrow-key navigation: handle ArrowLeft and ArrowRight on each tab, moving focus to the previous or next simulation mode with wraparound at either end. Preserve the existing button activation behavior and tab semantics.crates/qmrust-core/tests/properties.rs (1)
470-507: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert every paired report vector.
The test checks only
signal.signalandsv.noisy_signallengths.run_signalandrun_single_voxelexpose identities and curves as parallel vectors. A model-specific mismatch in those fields would pass this registry-wide test.Assert the identity, clean-signal, fitted-signal, and per-trial row dimensions against their paired fields.
Proposed coverage
assert!( signal.signal.iter().all(|v| v.is_finite()), "{name}: a noise-free forward signal must be finite", ); + assert_eq!( + signal.identities.len(), + signal.signal.len(), + "{name}: one identity per signal value", + ); // Every stats-reporting mode reports one row per parameter the fitter @@ assert_eq!( sv.noisy_signal.len(), model.n_volumes(), "{name}: the noisy signal covers every volume", ); + assert_eq!( + sv.clean_signal.len(), + sv.noisy_signal.len(), + "{name}: the clean signal must align with the noisy signal", + ); + assert_eq!( + sv.fitted_signal.len(), + sv.noisy_signal.len(), + "{name}: the fitted signal must align with the noisy signal", + ); + assert_eq!( + sv.identities.len(), + sv.noisy_signal.len(), + "{name}: one identity per noisy-signal value", + ); + for fitted in &sv.per_trial { + assert_eq!( + fitted.len(), + sv.fitted_names.len(), + "{name}: each fitted row must align with fitted_names", + ); + }🤖 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 `@crates/qmrust-core/tests/properties.rs` around lines 470 - 507, Extend the assertions around run_signal and run_single_voxel to validate every parallel report vector, not only signal.signal and sv.noisy_signal. Assert identity and clean-signal lengths against their paired fields, fitted-signal lengths against the corresponding clean-signal/identity data, and each sv.per_trial row dimension against its paired trial fields. Preserve the existing model.n_volumes(), estimated, and sim.trials expectations while adding these mismatch checks.
🤖 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 `@crates/qmrust-core/src/sim/noise.rs`:
- Around line 146-150: Correct the comment near the YAML-built configuration
test to state that adding a kind to NoiseKind::ALL makes it accepted by
configuration, rather than claiming enum variants are accepted automatically.
Preserve the existing explanation about deriving the accepted set from
NoiseKind::ALL and the serde-defaulted fields.
In `@docs/guide/simulation.md`:
- Around line 98-106: Complete the duplicated sentence by changing “whether or
not a dataset loaded” to “whether or not a dataset is loaded” in
docs/guide/simulation.md lines 98-106 and docs/playground.md lines 33-38.
In `@docs/playground/model.js`:
- Around line 230-233: Update the metadata construction consumed by
app.modelParams so bundle_slice() payloads retain params as names while also
emitting declared symbols with their units. Build the map from each
parameter-name string rather than reading p.name, and ensure every payload
includes the corresponding meta.symbols entries so model-specific labels and
distribution units are preserved.
- Line 234: Update the model-selection flow around seedSimRecipe(meta) so it
synchronizes editor.text with the newly selected simulation recipe before any
BIDS I/O or Simulate action can run. Ensure runSim() uses the selected model’s
recipe immediately, while preserving the existing app.simEditorText update.
In `@docs/playground/recipe.js`:
- Around line 563-565: Update syncFitArmed() so the `#fit` disabled state remains
true when WASM is unavailable, preserving the state set by loadModel() while
also applying the protocol-override condition. Ensure page-mode synchronization
cannot re-enable `#fit` when runSim() and fitSlice() are unsupported.
In `@docs/playground/sim.js`:
- Around line 671-673: Update the single-voxel note assigned to
$("sim-note").textContent to use singular “trial” when report.trials is 1 and
plural “trials” otherwise, in both occurrences within the message. Preserve the
existing wording and report.trials value.
In `@scripts/tests/icons.test.mjs`:
- Line 23: Update the icon-name extraction in the test around names so it
captures every quoted data-icon attribute value, including empty values and
values containing non-word characters, instead of restricting matches with \w-.
Keep the existing validation that each captured value resolves through
paintIcons/icon().
---
Nitpick comments:
In `@crates/qmrust-core/tests/properties.rs`:
- Around line 470-507: Extend the assertions around run_signal and
run_single_voxel to validate every parallel report vector, not only
signal.signal and sv.noisy_signal. Assert identity and clean-signal lengths
against their paired fields, fitted-signal lengths against the corresponding
clean-signal/identity data, and each sv.per_trial row dimension against its
paired trial fields. Preserve the existing model.n_volumes(), estimated, and
sim.trials expectations while adding these mismatch checks.
In `@docs/playground/sim.js`:
- Around line 155-177: In the failed-run branch of the simulation flow,
terminate and discard the cached worker before displaying the failure notice, so
the next ensureWorker call creates a fresh instance. Update the worker
reference/state used by ensureWorker alongside the termination, while preserving
the existing success and cancellation paths.
- Around line 739-754: Update buildModeTabs to add ARIA tab arrow-key
navigation: handle ArrowLeft and ArrowRight on each tab, moving focus to the
previous or next simulation mode with wraparound at either end. Preserve the
existing button activation behavior and tab semantics.
In `@scripts/make_docs_figures.py`:
- Around line 487-490: Consolidate the adjacent recipe comments into one block
with a single, correct statement that there are three recipes and a unified
explanation of their protocol and input differences. Remove the duplicated or
contradictory wording while preserving the existing recipe-specific details near
the config_sim definition.
🪄 Autofix
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: 5de0849d-dad5-432e-8750-1d4ff3c64e90
📒 Files selected for processing (61)
crates/qmrust-cli/src/catalog.rscrates/qmrust-core/src/config.rscrates/qmrust-core/src/core/model.rscrates/qmrust-core/src/registry.rscrates/qmrust-core/src/sim/mod.rscrates/qmrust-core/src/sim/noise.rscrates/qmrust-core/src/sim/report.rscrates/qmrust-core/tests/properties.rsdocs/agents/ARCHITECTURE.mddocs/guide/simulation.mddocs/models/field-mapping/b1_afi.mddocs/models/field-mapping/b1_dam.mddocs/models/magnetization-transfer/mt_ratio.mddocs/models/magnetization-transfer/mt_sat.mddocs/models/magnetization-transfer/qmt_spgr.mddocs/models/t1-relaxometry/inversion_recovery.mddocs/models/t1-relaxometry/vfa_t1.mddocs/models/t2-relaxometry/mono_t2.mddocs/playground.mddocs/playground/app.cssdocs/playground/app.jsdocs/playground/data/b1_afi.jsondocs/playground/data/b1_dam.jsondocs/playground/data/index.jsondocs/playground/data/inversion_recovery.jsondocs/playground/data/mono_t2.jsondocs/playground/data/mt_ratio.jsondocs/playground/data/mt_sat.jsondocs/playground/data/qmt_spgr.jsondocs/playground/data/vfa_t1.jsondocs/playground/dataset.jsdocs/playground/dom.jsdocs/playground/fit.jsdocs/playground/index.htmldocs/playground/inputs.jsdocs/playground/labels.jsdocs/playground/model.jsdocs/playground/recipe.jsdocs/playground/sim-series.jsdocs/playground/sim-worker.jsdocs/playground/sim.jsdocs/playground/skeleton.jsdocs/playground/state.jsdocs/playground/vendor/MANIFEST.jsondocs/playground/vendor/icons.jsrecipes/sim/b1_afi_sim.yamlrecipes/sim/b1_dam_sim.yamlrecipes/sim/irt1_sim.yamlrecipes/sim/mono_t2_sim.yamlrecipes/sim/mt_ratio_sim.yamlrecipes/sim/mt_sat_sim.yamlrecipes/sim/qmt_sim_ramani.yamlrecipes/sim/vfa_t1_sim.yamlscripts/check_source_hygiene.mjsscripts/make_docs_figures.pyscripts/tests/icons.test.mjsscripts/tests/labels.test.mjsscripts/tests/sim_series.test.mjsscripts/tests/skeleton.test.mjsscripts/tests/source_hygiene.test.mjsscripts/tests/test_dataset.py
🚧 Files skipped from review as they are similar to previous changes (43)
- docs/playground/data/qmt_spgr.json
- crates/qmrust-core/src/core/model.rs
- docs/playground/data/b1_dam.json
- docs/models/field-mapping/b1_afi.md
- docs/playground/data/mono_t2.json
- docs/playground/vendor/MANIFEST.json
- docs/playground/state.js
- scripts/tests/test_dataset.py
- docs/playground/data/b1_afi.json
- docs/playground/data/mt_sat.json
- docs/models/magnetization-transfer/mt_sat.md
- docs/models/t1-relaxometry/vfa_t1.md
- recipes/sim/b1_afi_sim.yaml
- recipes/sim/b1_dam_sim.yaml
- docs/models/field-mapping/b1_dam.md
- docs/models/t2-relaxometry/mono_t2.md
- docs/models/magnetization-transfer/qmt_spgr.md
- recipes/sim/mt_sat_sim.yaml
- recipes/sim/mt_ratio_sim.yaml
- recipes/sim/vfa_t1_sim.yaml
- docs/playground/inputs.js
- recipes/sim/irt1_sim.yaml
- docs/playground/index.html
- docs/models/magnetization-transfer/mt_ratio.md
- crates/qmrust-core/src/config.rs
- docs/playground/fit.js
- docs/playground/data/inversion_recovery.json
- crates/qmrust-cli/src/catalog.rs
- docs/playground/data/index.json
- recipes/sim/qmt_sim_ramani.yaml
- scripts/tests/sim_series.test.mjs
- docs/playground/data/mt_ratio.json
- docs/models/t1-relaxometry/inversion_recovery.md
- docs/playground/app.js
- crates/qmrust-core/src/registry.rs
- recipes/sim/mono_t2_sim.yaml
- scripts/check_source_hygiene.mjs
- crates/qmrust-core/src/sim/report.rs
- scripts/tests/source_hygiene.test.mjs
- docs/playground/sim-series.js
- docs/agents/ARCHITECTURE.md
- docs/playground/sim-worker.js
- crates/qmrust-core/src/sim/mod.rs
| app.modelParams = new Map( | ||
| (meta.params ?? []).map((p) => [p.name, declaredUnits.get(p.name) ?? null]), | ||
| ); | ||
| seedSimRecipe(meta); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Load the selected simulation recipe before BIDS I/O.
When Simulate mode is active and a user selects another model, seedSimRecipe(meta) updates only app.simEditorText. editor.text keeps the previous model recipe until Line 69 or Line 275. The Simulate action remains available in that interval, so runSim() can execute the previous model recipe under the new model selection.
Proposed fix
app.modelParams = new Map(
(meta.params ?? []).map((p) => [p.name, declaredUnits.get(p.name) ?? null]),
);
seedSimRecipe(meta);
+ if (isSimMode()) setEditorText(app.simEditorText);
app.wheelAccum = 0;🤖 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/playground/model.js` at line 234, Update the model-selection flow around
seedSimRecipe(meta) so it synchronizes editor.text with the newly selected
simulation recipe before any BIDS I/O or Simulate action can run. Ensure
runSim() uses the selected model’s recipe immediately, while preserving the
existing app.simEditorText update.
Comment on lines
+563
to
+565
| const blocked = protocolLockApplies() && Boolean(app.overrideProtocol); | ||
| fit.disabled = blocked; | ||
| fit.title = blocked |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep #fit disabled when WASM is unavailable.
When WASM fails to load, loadModel() disables #fit. A page-mode switch calls syncFitArmed(), and this code replaces that state with only the protocol-override condition. The action then becomes enabled although runSim() and fitSlice() immediately reject it.
Proposed fix
export function syncFitArmed() {
const fit = $("fit");
if (!fit) return;
+ const unavailable = !app.wasm;
const blocked = protocolLockApplies() && Boolean(app.overrideProtocol);
- fit.disabled = blocked;
- fit.title = blocked
+ fit.disabled = unavailable || blocked;
+ fit.title = unavailable
+ ? "WASM unavailable"
+ : blocked
? "Arm the protocol inputs to fit: edited values cannot be fitted against a BIDS dataset"
: "";
}📝 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
| const blocked = protocolLockApplies() && Boolean(app.overrideProtocol); | |
| fit.disabled = blocked; | |
| fit.title = blocked | |
| const unavailable = !app.wasm; | |
| const blocked = protocolLockApplies() && Boolean(app.overrideProtocol); | |
| fit.disabled = unavailable || blocked; | |
| fit.title = unavailable | |
| ? "WASM unavailable" | |
| : blocked | |
| ? "Arm the protocol inputs to fit: edited values cannot be fitted against a BIDS dataset" | |
| : ""; |
🤖 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/playground/recipe.js` around lines 563 - 565, Update syncFitArmed() so
the `#fit` disabled state remains true when WASM is unavailable, preserving the
state set by loadModel() while also applying the protocol-override condition.
Ensure page-mode synchronization cannot re-enable `#fit` when runSim() and
fitSlice() are unsupported.
Comment on lines
+671
to
+673
| $("sim-note").textContent = | ||
| `The first of ${report.trials} noisy trials, against the truth it was ` | ||
| + `generated from and the curve fitted back from it. The table summarises all ${report.trials} trials.`; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Pluralize the trial count in the single-voxel note.
A recipe can set trials: 1. The note then reads "The first of 1 noisy trials" and "all 1 trials". The montecarlo note at line 695 already pluralizes "voxel". Apply the same rule here.
✏️ Proposed change
- $("sim-note").textContent =
- `The first of ${report.trials} noisy trials, against the truth it was `
- + `generated from and the curve fitted back from it. The table summarises all ${report.trials} trials.`;
+ const trial = `trial${report.trials === 1 ? "" : "s"}`;
+ $("sim-note").textContent =
+ `The first of ${report.trials} noisy ${trial}, against the truth it was `
+ + `generated from and the curve fitted back from it. The table summarises all `
+ + `${report.trials} ${trial}.`;📝 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
| $("sim-note").textContent = | |
| `The first of ${report.trials} noisy trials, against the truth it was ` | |
| + `generated from and the curve fitted back from it. The table summarises all ${report.trials} trials.`; | |
| const trial = `trial${report.trials === 1 ? "" : "s"}`; | |
| $("sim-note").textContent = | |
| `The first of ${report.trials} noisy ${trial}, against the truth it was ` | |
| `generated from and the curve fitted back from it. The table summarises all ` | |
| `${report.trials} ${trial}.`; |
🤖 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/playground/sim.js` around lines 671 - 673, Update the single-voxel note
assigned to $("sim-note").textContent to use singular “trial” when report.trials
is 1 and plural “trials” otherwise, in both occurrences within the message.
Preserve the existing wording and report.trials value.
| // `paintIcons` resolves these at startup, so one bad name throws before the | ||
| // page finishes wiring. | ||
| const html = readFileSync(new URL("../../docs/playground/index.html", import.meta.url), "utf8"); | ||
| const names = new Set([...html.matchAll(/data-icon="([\w-]+)"/g)].map((m) => m[1])); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Match every data-icon attribute value.
Line 23 skips empty values and values with unsupported characters. For example, data-icon="umbrella!" is not tested, but paintIcons can still pass it to icon() and fail page startup. Capture every quoted value, then require each value to resolve.
Proposed fix
- const names = new Set([...html.matchAll(/data-icon="([\w-]+)"/g)].map((m) => m[1]));
+ const names = new Set(
+ [...html.matchAll(/\bdata-icon\s*=\s*(["'])(.*?)\1/g)].map((m) => m[2]),
+ );📝 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
| const names = new Set([...html.matchAll(/data-icon="([\w-]+)"/g)].map((m) => m[1])); | |
| const names = new Set( | |
| [...html.matchAll(/\bdata-icon\s*=\s*(["'])(.*?)\1/g)].map((m) => m[2]), | |
| ); |
🤖 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 `@scripts/tests/icons.test.mjs` at line 23, Update the icon-name extraction in
the test around names so it captures every quoted data-icon attribute value,
including empty values and values containing non-word characters, instead of
restricting matches with \w-. Keep the existing validation that each captured
value resolves through paintIcons/icon().
…y their units Claude-Session: https://claude.ai/code/session_01DSdowSspKCv3nE5sN2WirM
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/playground/data/vfa_t1.json (1)
143-144: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winValidate the parsed config_sim objects before relying on these snippets.
config_simremains a YAML text field in these playground data files, so the listed acquisition, fitting, simulation parameters, sweep, and distribution contents need a Rust/WASM validation path instead of ad hoc parsing.🤖 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/playground/data/vfa_t1.json` around lines 143 - 144, Validate each YAML string stored in config_sim through the existing Rust/WASM configuration-validation path before relying on its acquisition, fitting, simulation, sweep, and distribution fields. Apply the same validation to the affected entries in docs/playground/data/vfa_t1.json (143-144), docs/playground/data/b1_afi.json (120-121), docs/playground/data/b1_dam.json (120-121), docs/playground/data/mt_ratio.json (110-111), docs/playground/data/mt_sat.json (156-157), and docs/playground/data/qmt_spgr.json (290-291); replace any ad hoc parsing with the shared validator.
🤖 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/playground/sim.js`:
- Around line 46-50: Update the Simulate-mode model-loading flow around
seedSimRecipe, loadModelFromBids, and setEditorText so model selection and
Simulate cannot run while the archive is loading, or ensure user edits are
preserved and the post-load editor sync never overwrites them. Keep the editor
synchronized with app.simEditorText only when no user edit occurred during
loading.
---
Outside diff comments:
In `@docs/playground/data/vfa_t1.json`:
- Around line 143-144: Validate each YAML string stored in config_sim through
the existing Rust/WASM configuration-validation path before relying on its
acquisition, fitting, simulation, sweep, and distribution fields. Apply the same
validation to the affected entries in docs/playground/data/vfa_t1.json
(143-144), docs/playground/data/b1_afi.json (120-121),
docs/playground/data/b1_dam.json (120-121), docs/playground/data/mt_ratio.json
(110-111), docs/playground/data/mt_sat.json (156-157), and
docs/playground/data/qmt_spgr.json (290-291); replace any ad hoc parsing with
the shared validator.
🪄 Autofix
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: b6a6bc90-6e59-4b37-a643-d7f52e0d6d23
📒 Files selected for processing (15)
crates/qmrust-core/src/sim/noise.rsdocs/guide/simulation.mddocs/playground.mddocs/playground/data/b1_afi.jsondocs/playground/data/b1_dam.jsondocs/playground/data/inversion_recovery.jsondocs/playground/data/mono_t2.jsondocs/playground/data/mt_ratio.jsondocs/playground/data/mt_sat.jsondocs/playground/data/qmt_spgr.jsondocs/playground/data/vfa_t1.jsondocs/playground/model.jsdocs/playground/sim.jsscripts/make_docs_figures.pyscripts/tests/test_dataset.py
🚧 Files skipped from review as they are similar to previous changes (7)
- docs/playground/model.js
- docs/playground/data/mono_t2.json
- scripts/make_docs_figures.py
- crates/qmrust-core/src/sim/noise.rs
- scripts/tests/test_dataset.py
- docs/playground/data/inversion_recovery.json
- docs/guide/simulation.md
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.
Wires the core's four simulation modes into the playground, then brings two of
their charts onto qMRLab's own conventions.
The core already implemented
signal,single-voxel,sensitivityandmontecarloand already exported them to wasm assim(mode, cfg_yaml). Nothingin
docs/playground/called it. Simulation is now a page mode of the playgroundfor every registered model, reading no image data, so it works whether or not a
dataset loaded.
The conventions being followed
Read from a qMRLab checkout, not from memory.
src/Models/T1_relaxometry/inversion_recovery.mdeclares the family, and eachof our modes names its counterpart in
docs/guide/simulation.md:single-voxelSim_Single_Voxel_CurvesensitivitySim_Sensitivity_AnalysismontecarloSim_Multi_Voxel_Distributionsignalhas no counterpart, being a noise-free forward signal on its own.Sim_Optimize_Protocolis declared upstream and not implemented here.src/Addons/SimVary/SimVaryPlot.mis why Sensitivity plots fitted against inputin physical units, with a diagonal identity line for the swept parameter and a
horizontal truth line for the others.
src/Addons/SimRnd/SimRndPlotResults.moffers eight views; Multi-Voxel charts its
Input vs. Fitscatter and itsErrorhistogram. ItsRMSE,NRMSEandMPEcases are one number perparameter and the stats table already reports RMSE numerically.
Core changes
Recipes.simis no longer anOption. Every model implements a forwardsignal, so every model is simulatable; an
Optionmodelled an impossibilityand let a model silently drop out of Simulate mode. Seven recipes were written
to fill it, and
tests/properties.rsnow runs all four modes for everyregistered model off its declared recipe, so a model added later is covered
with no per-model line.
NoiseKindis the one home for thesim.noise.typenames.SimConfig::validatedelegated to a second copy ofnone|gaussian|rician,and the playground would have been a third. The catalog emits the list and the
payload index carries it to the browser, so the dropdown derives rather than
restates.
core already computed and discarded rather than anything newly derived.
SingleVoxelReportgainedclean_signalandfitted_signal, so the CLI's SVGplot path reads them instead of rebuilding them.
SignalReportandSingleVoxelReportgainedidentities, reusingVolumeId's existingencoding.
MonteCarloReportgainedper_trial_inputandper_trial_fitted;the error is their difference and is not stored beside them.
resolve_simis the one precondition for a simulation. Validation livedonly in the CLI's
run_sim, so the browser path reached the fourrun_*functions with none. Now each of them enforces it and the CLI's copy is gone,
which also removed a pre-existing double model build in the sweep and
montecarlo paths.
What review caught
Worth reading before approving, since most of these were defects where the data
was right and the presentation was not.
fit as JSON
null, and in JavaScriptnull - inputis exactly-inputand isfinite, so a
Number.isFinitecheck applied after the subtraction passed itthrough. With
vfa_t1_sim.yamlatsnr: 3, trial 8 diverges and the chartinvented an
M0error of exactly-1000while the stats table honestly showedNaN. The fabricated point also rescaled both scatter axes, becauseMath.minreadsnullas 0. Fixed by testing operands before arithmeticrather than results afterwards, and the dropped trial is now reported.
trials: 0indexedper_trial[0]on an empty vector andsnr: 0madesigma_forreturn infinity, whichNormal::newrejects at an.expect(). Awasm panic traps the module and the worker caches its instance, so the tab was
dead until reload. This is what
resolve_simabove is for.hiddendid nothing on five cards. It hides via the UA sheet's[hidden] { display: none }at specificity 0,1,0, and every one of those cardsset
display: flexin a class rule, which defeats it. The Simulation card wastherefore always rendered, covering the Inputs and Fitted map panels and
swallowing their clicks.
app.cssalready documented this exact trap forother components. A
hiddenDisplayGapsrule incheck_source_hygiene.mjsnowguards the class of bug, and its test reintroduces the fault so the checker has
been watched failing.
dataset metadata while plotting an edited recipe, an uncertainty band drawn
from zero rather than straddling the line because ECharts stacks by sign, a
shared axis that flattened every parameter but the largest, an axis named for a
parameter while plotting its error, error bars clipped by the panel border so
uncertainty read as smaller than measured, and reference lines drawn outside
the panel they belonged to. The last of those is worth a look: with
mono_t2at
snr: 5, restoring the identity line reveals fitted T2 sitting far aboveit, a catastrophic fit failure that was invisible while the line was clipped
away.
mt_sat's recipe setMTSAT: 0.03, treating MT saturation as a fraction, but the model carries itin percent:
fit.rsdivides by 100 and the registry declares the unit as%.At 0.03 the simulated MT effect was essentially absent.
Deferred
Recorded rather than fixed, none load-bearing.
irt1_sim.yamldraws onlyT1from a distribution, so Multi-Voxel'saandbscatters collapse to vertical lines. Honest, but two of three panels carryno input-output information. Giving those amplitudes distributions is a qMRI
ground-truth decision.
sim.jsrestatescurve.js's theme-token read, and the sensitivity chartborrows
draw.js'sLABELSpalette for series colours, coupling asegmentation taxonomy to chart colours. Both are second occurrences, so the
Rule of Three says wait; a third chart should trigger extraction of a
categorical-colour module and a shared chart-option builder.
docs/guide/simulation.mdsays the modes "wrap" qMRLab's methods, where"correspond to" is what the rest of the file accurately says.
Verification
cargo test --workspace,cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings,cargo build -p qmrust-core --target wasm32-unknown-unknown,node --test scripts/tests/*.test.mjs(178),node scripts/check_source_hygiene.mjs,node scripts/check_theme_contrast.mjs,and
python3 -m unittest discover -s scripts/tests(26). All green.Behaviour preservation was held to byte identity rather than to a tolerance: the
CLI's single-voxel SVG plot is byte-for-byte identical across the report change
that fed it, and every pre-existing report field was compared before and after
by building the parent commit in a scratch worktree.
https://claude.ai/code/session_01DSdowSspKCv3nE5sN2WirM
Summary by CodeRabbit
New Features
Bug Fixes
Documentation