Add mtr, mtsat, and simulation based correction - #8
Merged
Conversation
added 18 commits
July 23, 2026 18:24
MTR is the first Named model (roles MTon/MToff, no protocol, closed-form fit). Generalizes the shell so Named collections fit: load_collection stacks role volumes in the model's declared order, and bidsify reads one <role>.mat per role. Grouping named_set role names must match measurement() roles. Validated bit-exact against qMRLab FitResults; wired into integration_osf.sh. Claude-Session: https://claude.ai/code/session_013BCm4Xwt1hFmr1ukQgV1JV
Porting a model requires choosing a BIDS grouping suffix and per-volume entities; the qMRI appendix is the authority. Add it as a gate question and a pointer so ports prefer canonical suffixes over inventing them. Claude-Session: https://claude.ai/code/session_013BCm4Xwt1hFmr1ukQgV1JV
MTsat (Helms 2008): a Named three-role model (MTw/PDw/T1w = BIDS MTS) with an optional B1map aux and a closed-form, T1-corrected fit outputting MTSAT/T1 and (when TR_MT==TR_PD) MTR. Generalizes the core so a Named measurement can also carry a sidecar-resolved per-role protocol: validate_against_protocol accepts one row per role, load_collection reorders a named set's per-role protocol to the model's role order, and bidsify reads a per-role NIfTI source (--nii-dir). No-B1 path validated against qMRLab FitResults to double-precision round-off (MTSAT/T1/MTR); B1 correction branch implemented but unvalidated (no B1map reference data). Wired into integration_osf.sh. Claude-Session: https://claude.ai/code/session_013BCm4Xwt1hFmr1ukQgV1JV
The BIDS flip-1/flip-2 labels only order the MTS volumes; T1w is defined as the higher-flip-angle image. Rather than trust the labels, mt_sat assigns PD/T1 by the resolved flip-angle value, keeping each signal paired with its own FA/TR. The Helms R1/A/MTsat are invariant to the PDw<->T1w swap, so those maps are correct either way; MTR needs the true PD reference, which the reassignment supplies. A mislabeled pair is corrected with a warning (not a hard failure); only an equal-flip-angle pair is genuinely ambiguous and is rejected. Guards both the BIDS-sidecar and config paths. Claude-Session: https://claude.ai/code/session_013BCm4Xwt1hFmr1ukQgV1JV
… in the same coordinates (fixes C1) C1: calibration (collect_mtsat_r1_b1) and correction (MtSatModel::fit, Tardif active) fed the M0b-vs-R1 line inconsistent R1/MTsat definitions because fit::mtsat() bundled B1-correcting R1/A together with applying the empirical Helms multiplicative factor to MTsat under one b1 argument. fit::mtsat now takes an explicit apply_helms bool, decoupled from B1-correction of R1/A. MtSatModel gains a calibration_mode (via new_calibration/build_calibration) in which fit() emits raw (pre-Helms, pre-CF) MTsat plus B1-corrected R1, the same coordinates the Tardif correction path uses, while the plain no-fitvalues path keeps applying the empirical Helms factor (qMRLab-faithful). collect_mtsat_r1_b1 now builds via build_calibration so calibration and correction agree exactly. As a side effect, T1 is now B1-corrected whenever a B1 map is present in every path, including Tardif (previously an apparent/uncorrected T1). M1: inject_mt_sat_b1_correction now warns (not errors) when a recipe's b1_correction.b1_ref disagrees with the loaded fitvalues artifact's own b1_ref, naming both; the artifact's value remains authoritative. Recipe comment updated to say so. Claude-Session: https://claude.ai/code/session_013BCm4Xwt1hFmr1ukQgV1JV
|
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:
📝 WalkthroughWalkthroughAdds ChangesMT model contracts and BIDS integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…ation) The mtsat-b1 self-calibration only needs MTsat + T1; requiring TR_MT == TR_PD (the MTR-export constraint) wrongly rejected protocols whose PD-weighted VFA volume has a different TR than the MT-weighted volume (e.g. TardifLab data: MTw 28 ms, PDw/T1w 30 ms). Surfaced by an end-to-end run on real Tardif inputs. Claude-Session: https://claude.ai/code/session_013BCm4Xwt1hFmr1ukQgV1JV
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (8)
ci/integration_osf.sh (2)
93-100: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExercise the new B1-correction path in integration coverage.
This run uses
recipes/bids/mt_sat_config.yamland does not invokemtsat-b1ormt_sat_b1corr_config.yaml, so simulation, artifact injection, and corrected MTsat fitting can regress without failing OSF integration. Add a corrected-path fixture/run or a dedicated end-to-end test.🤖 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 `@ci/integration_osf.sh` around lines 93 - 100, Extend the OSF integration coverage around the existing mt_sat bidsify/fit flow to exercise the B1-correction path: use the relevant mtsat-b1 operation and mt_sat_b1corr_config.yaml, including simulation and artifact injection as required by that path, then run the corrected MTsat fit and assert it succeeds. Keep the current uncorrected fit coverage intact and anchor the additions to the existing bidsify and fit commands.
43-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate every input consumed by the new CI steps.
The MTR checks locate only
MTon.matand its reference, while bidsify also requiresMToff.matandMask.mat. Likewise, the MTsat checks validate onlyMTw.nii.gzandMTSAT.nii.gz, but later commands consumePDw.nii.gz,T1w.nii.gz,T1.nii.gz, andMTR.nii.gz. Add explicit checks so archive drift fails with the intended diagnostic.🤖 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 `@ci/integration_osf.sh` around lines 43 - 60, Validate every file consumed by the MTR and MTsat CI flows. In the MTR setup around MTR_ON and MTR_REF, add existence checks for MToff.mat and Mask.mat with clear archive-specific diagnostics; in the MTsat setup around MTSAT_MTW and the reference variables, add checks for PDw.nii.gz, T1w.nii.gz, FitResults/T1.nii.gz, and FitResults/MTR.nii.gz. Preserve the existing find-based discovery and fail immediately when any required file is missing.crates/qmrust-core/src/mtsat_b1/correct.rs (1)
1-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc comment cites implementation provenance rather than behavior/contract.
"ported from
<ref>/functions/MTsat_B1corr_factor_map.m" is a historical/provenance note, not a description of what the function does or its invariant.As per coding guidelines, "Comments and docstrings must explain what code does and the invariant or contract behind it, not authorship, alternatives, historical narrative, reviewer asides, or task references."
✏️ Proposed rewording
-//! The voxelwise MTsat B1 correction factor, ported from -//! <ref>/functions/MTsat_B1corr_factor_map.m: estimate M0b from R1, evaluate -//! the surface at the achieved vs nominal saturation amplitude, and form the -//! relative change. `MTsat_corr = MTsat·(1 + CF)`. +//! The voxelwise MTsat B1 correction factor: estimate M0b from R1, evaluate +//! the fitted saturation surface at the achieved vs nominal saturation +//! amplitude, and form the relative change. `MTsat_corr = MTsat·(1 + CF)`.🤖 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/src/mtsat_b1/correct.rs` around lines 1 - 4, Update the module doc comment in the MTsat B1 correction implementation to remove the MATLAB file provenance reference and describe only the correction’s behavior and contract: estimate M0b from R1, evaluate the saturation-surface change between achieved and nominal amplitudes, and return the relative correction factor used by MTsat.Source: Coding guidelines
crates/qmrust-cli/src/commands.rs (1)
349-379: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlphabetical role-order zip for
Namedprotocol resolution has no test with a non-empty schema.The reorder logic assumes
resolve_protocolreturns aNamedcollection's per-volume protocol in the same alphabetical-by-role-key order asmap.keys(). This is a subtle, correctness-critical assumption (misalignment here would silently swap FlipAngle/TR values between roles likeMTw/PDw/T1w), but the only test exercisingNamedcollections (load_collection_named_needs_role_axis_and_all_roles, lines 1562-1589) uses an empty schema, soProtocol::default()short-circuits before this branch ever runs.Consider adding a test with a non-empty schema (e.g. a synthetic
NamedMTS/MTR fixture with distinct per-role sidecar values) assertingproto.volumes[i]matchesroles[i]'s sidecar, to lock in this contract withrust_bids::resolve_protocol.🤖 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-cli/src/commands.rs` around lines 349 - 379, Add a test covering the Named collection path with a non-empty schema and distinct per-role sidecar protocol values, using the existing test helpers and symbols near load_collection_named_needs_role_axis_and_all_roles. Assert that the resolved proto.volumes entries follow the declared roles order and retain each role’s own sidecar values, exercising the resolve_protocol result and reordering logic rather than the empty-schema default path.crates/qmrust-cli/src/io/mat.rs (1)
258-291: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate named-role-stacking logic between the
.matand NIfTI readers.read_named_mat_volumesandread_named_nii_volumesboth implement the identical "read one file per role → validate matching spatial dims → stack intoArray4in role order" algorithm, differing only in the per-file reader used.
crates/qmrust-cli/src/io/mat.rs#L258-L291: extract the dims-validation/stacking loop into a shared generic helper (e.g. taking aFn(&Path) -> Result<Array3<f64>>reader) that this function calls withread_map_mat.crates/qmrust-cli/src/io/nifti.rs#L112-L149: reuse the same shared helper, wrappingread_map_nifti_with_headerto also thread through the header capture.🤖 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-cli/src/io/mat.rs` around lines 258 - 291, Extract the shared per-role reading, spatial-dimension validation, and Array4 stacking logic from read_named_mat_volumes into a reusable generic helper accepting a path reader function; update crates/qmrust-cli/src/io/mat.rs:258-291 to call it with read_map_mat, and crates/qmrust-cli/src/io/nifti.rs:112-149 to reuse it while preserving read_map_nifti_with_header header capture.crates/qmrust-core/src/mtsat_b1/pulse.rs (1)
69-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGyromagnetic-ratio constant (
GAMMA) is duplicated with drift acrosspulse.rsandsim.rs.sim.rsdefinesconst GAMMA: f64 = 42.57747892;;pulse.rsindependently hardcodes42.57748for the same physical constant instead of importing it — the two values already differ in the trailing digits.
crates/qmrust-core/src/mtsat_b1/pulse.rs#L69-L85: replace the inline42.57748insinc_exc_pulsewith the shared constant (e.g. definepub(crate) const GAMMAhere, sincesim.rsalready depends onpulse.rs).crates/qmrust-core/src/mtsat_b1/sim.rs#L40-L40: drop the localconst GAMMAdefinition anduse crate::mtsat_b1::pulse::GAMMA;instead.🤖 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/src/mtsat_b1/pulse.rs` around lines 69 - 85, Centralize the gyromagnetic-ratio constant by defining pub(crate) GAMMA in sinc_exc_pulse’s pulse module and using it in the w_b1 calculation instead of the inline value. In crates/qmrust-core/src/mtsat_b1/sim.rs:40-40, remove the local GAMMA definition and import pulse::GAMMA; no other behavior should change.crates/qmrust-core/src/mtsat_b1/sim.rs (1)
96-124: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy lift
propagaterecomputes the full lineshape sum on every discretized step instead of once per pulse.
rate_matrix(and, forSuperLorentzian, its 1000-iterationabsorptionsum) is rebuilt from scratch on every call inpropagate, even thoughdelta/t2b/lineshape are constant across all steps within a saturation pulse (onlyw1varies with the pulse envelope). Givensurface::fit's calibration grid requires a fullmamt_signal/vfa_apparentrun (hundreds of TR loops × pulse steps) per sample point, this redundant recomputation could add up to a substantial chunk of calibration runtime. Consider hoisting the delta-dependent lineshape/wlocterm out of the per-step loop and only rescaling byw1²per step.🤖 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/src/mtsat_b1/sim.rs` around lines 96 - 124, The propagate closure currently rebuilds rate_matrix, including the delta-dependent lineshape calculation, for every pulse step. Refactor the saturation-pulse setup around rate_matrix and propagate so the constant delta/t2b/lineshape-derived wloc term is computed once per pulse, while each step only applies the current w1² scaling before propagating; preserve the existing magnetization and envelope behavior.crates/qmrust-core/src/mtsat_b1/surface.rs (1)
8-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winManual 64-element (de)serializer could be replaced by a small helper crate.
This is a correct workaround for serde's current cap on built-in array (de)serialization (32 elements), but it's ~40 lines of hand-rolled
Serializer/Deserializer/Visitorboilerplate.serde_arrays(no_std-compatible) orserde-big-arraywould let#[serde(with = "...")]on thecoeffsfield replace this entirely, keeping the wasm-clean constraint forqmrust-corewhile removing custom serialization code. Please confirm the current serde version in use doesn't already support this natively via const generics before adopting a dependency.🤖 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/src/mtsat_b1/surface.rs` around lines 8 - 52, The manual Serialize and Deserialize implementations for SsSurface should be replaced with a serde array helper if the current serde version lacks native support for 64-element arrays. Confirm serde’s version and array support first, then add an appropriate no_std-compatible helper dependency and annotate SsSurface::coeffs with its serde adapter, removing the custom serializer and CoeffsVisitor while preserving fixed-length 64-element tuple behavior.
🤖 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 `@ci/integration_osf.sh`:
- Around line 122-143: Update the explanatory comment above the mt_sat
comparisons to describe the no-B1 case as the “uncorrected path,” removing the
inaccurate “uncorrected Helms path” wording while leaving the comparison
commands unchanged.
In `@crates/qmrust-cli/src/bidsify.rs`:
- Around line 178-197: Update read_named_nifti_source to load every
model.required_inputs() auxiliary map from dir, mirroring read_mat_source, and
return those loaded inputs instead of Vec::new(). Preserve the existing
role-volume, mask, and header handling while ensuring aux files such as
B1map.nii.gz are included in the resulting Source.
In `@crates/qmrust-cli/src/commands.rs`:
- Around line 438-441: Replace the per-model cfg.model == "mt_sat" checks in
run_fit and run_fit_bids with ModelEntry dispatch. Add an optional inject_extras
function pointer to ModelEntry, register inject_mt_sat_b1_correction there,
resolve the entry before recipe configuration, and invoke its injector when
present so CLI code does not branch on model names.
In `@crates/qmrust-cli/src/mtsat_b1.rs`:
- Around line 52-55: Validate self.grid.b1_steps before constructing b1s in the
surrounding grid-building method, requiring at least two steps and returning the
established error type for invalid configuration. Only perform the division in
the existing map after validation, preserving the current grid generation for
valid step counts.
In `@crates/qmrust-core/src/mtsat_b1/correct.rs`:
- Around line 9-14: Update correction_factor to guard the denominator ss_act
before computing (ss_nom - ss_act) / ss_act; when ss_act is zero or sufficiently
close to zero, return a small safe fallback or use an epsilon denominator so the
result never becomes Inf or NaN.
In `@crates/qmrust-core/src/mtsat_b1/fitvalues.rs`:
- Around line 44-64: Update fitvalues_yaml_roundtrips to initialize
SsSurface.coeffs with distinct, non-zero values rather than [0.0; 64], then
assert that back.ss_surface.coeffs matches the original full 64-element array.
Keep the existing b1_ref and m0b_vs_r1 assertions.
In `@crates/qmrust-core/src/mtsat_b1/lineshape.rs`:
- Around line 57-83: Replace the self-derived expected values in
super_lorentzian_matches_reference_magnitude and gaussian_uses_angular_offset
with independently computed literal fixture values for the documented t2b and
delta inputs. Keep assertions focused on matching those fixed reference outputs
within an appropriate tolerance, retaining only sanity checks that do not
reimplement the production formulas.
In `@crates/qmrust-core/src/mtsat_b1/sim.rs`:
- Around line 56-65: Update the `ra` calculation’s fallback condition to reject
all non-finite values, including positive and negative infinity, rather than
checking only `val.is_nan()`. Preserve the existing `1.0` fallback for invalid
results and the calculated value for finite results.
- Around line 86-88: Validate the computed tr_fill in the simulation setup
before it reaches propagate: if the configured pulse and excitation durations
exceed p.tr, reject the configuration with an explicit error rather than
allowing a negative integration time. Preserve normal propagation for
non-negative tr_fill and anchor the validation to the tr_fill calculation and
propagate call.
- Around line 136-142: Update the water-excitation loop in the simulation flow
to convert the µT amplitude from sinc_exc_pulse() to rad/s before passing it to
propagate(), matching the saturation pulse conversion by including the 2π factor
in the w1 calculation. Preserve the existing indexing, excitation repetition,
and propagation behavior.
In `@docs/agents/ARCHITECTURE.md`:
- Around line 51-54: Add the missing mt_ratio sibling entry to the architecture
tree in ARCHITECTURE.md, using the {config,fit,model}.rs structure shown for
mt_sat. Keep the documented qmrust-core layout synchronized without changing
unrelated entries.
In `@recipes/bids/mt_sat_b1corr_config.yaml`:
- Around line 16-18: The MTsat B1 calibration reference must be consistent
across both recipes: update b1_ref in recipes/bids/mt_sat_b1corr_config.yaml
lines 16-18 and the generated-artifact reference in recipes/mtsat_b1_seq.yaml
line 34 to the same intended value, preserving the producer/consumer
configuration alignment.
---
Nitpick comments:
In `@ci/integration_osf.sh`:
- Around line 93-100: Extend the OSF integration coverage around the existing
mt_sat bidsify/fit flow to exercise the B1-correction path: use the relevant
mtsat-b1 operation and mt_sat_b1corr_config.yaml, including simulation and
artifact injection as required by that path, then run the corrected MTsat fit
and assert it succeeds. Keep the current uncorrected fit coverage intact and
anchor the additions to the existing bidsify and fit commands.
- Around line 43-60: Validate every file consumed by the MTR and MTsat CI flows.
In the MTR setup around MTR_ON and MTR_REF, add existence checks for MToff.mat
and Mask.mat with clear archive-specific diagnostics; in the MTsat setup around
MTSAT_MTW and the reference variables, add checks for PDw.nii.gz, T1w.nii.gz,
FitResults/T1.nii.gz, and FitResults/MTR.nii.gz. Preserve the existing
find-based discovery and fail immediately when any required file is missing.
In `@crates/qmrust-cli/src/commands.rs`:
- Around line 349-379: Add a test covering the Named collection path with a
non-empty schema and distinct per-role sidecar protocol values, using the
existing test helpers and symbols near
load_collection_named_needs_role_axis_and_all_roles. Assert that the resolved
proto.volumes entries follow the declared roles order and retain each role’s own
sidecar values, exercising the resolve_protocol result and reordering logic
rather than the empty-schema default path.
In `@crates/qmrust-cli/src/io/mat.rs`:
- Around line 258-291: Extract the shared per-role reading, spatial-dimension
validation, and Array4 stacking logic from read_named_mat_volumes into a
reusable generic helper accepting a path reader function; update
crates/qmrust-cli/src/io/mat.rs:258-291 to call it with read_map_mat, and
crates/qmrust-cli/src/io/nifti.rs:112-149 to reuse it while preserving
read_map_nifti_with_header header capture.
In `@crates/qmrust-core/src/mtsat_b1/correct.rs`:
- Around line 1-4: Update the module doc comment in the MTsat B1 correction
implementation to remove the MATLAB file provenance reference and describe only
the correction’s behavior and contract: estimate M0b from R1, evaluate the
saturation-surface change between achieved and nominal amplitudes, and return
the relative correction factor used by MTsat.
In `@crates/qmrust-core/src/mtsat_b1/pulse.rs`:
- Around line 69-85: Centralize the gyromagnetic-ratio constant by defining
pub(crate) GAMMA in sinc_exc_pulse’s pulse module and using it in the w_b1
calculation instead of the inline value. In
crates/qmrust-core/src/mtsat_b1/sim.rs:40-40, remove the local GAMMA definition
and import pulse::GAMMA; no other behavior should change.
In `@crates/qmrust-core/src/mtsat_b1/sim.rs`:
- Around line 96-124: The propagate closure currently rebuilds rate_matrix,
including the delta-dependent lineshape calculation, for every pulse step.
Refactor the saturation-pulse setup around rate_matrix and propagate so the
constant delta/t2b/lineshape-derived wloc term is computed once per pulse, while
each step only applies the current w1² scaling before propagating; preserve the
existing magnetization and envelope behavior.
In `@crates/qmrust-core/src/mtsat_b1/surface.rs`:
- Around line 8-52: The manual Serialize and Deserialize implementations for
SsSurface should be replaced with a serde array helper if the current serde
version lacks native support for 64-element arrays. Confirm serde’s version and
array support first, then add an appropriate no_std-compatible helper dependency
and annotate SsSurface::coeffs with its serde adapter, removing the custom
serializer and CoeffsVisitor while preserving fixed-length 64-element tuple
behavior.
🪄 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: 09a0610c-03b5-4596-a823-47ef90f7ef8b
📒 Files selected for processing (42)
.claude/skills/porting-qmrlab-models/SKILL.mdci/integration_osf.shcrates/qmrust-cli/src/bidsify.rscrates/qmrust-cli/src/commands.rscrates/qmrust-cli/src/io/mat.rscrates/qmrust-cli/src/io/nifti.rscrates/qmrust-cli/src/main.rscrates/qmrust-cli/src/mtsat_b1.rscrates/qmrust-core/src/core/model.rscrates/qmrust-core/src/engine.rscrates/qmrust-core/src/lib.rscrates/qmrust-core/src/models/mod.rscrates/qmrust-core/src/models/mt_ratio/config.rscrates/qmrust-core/src/models/mt_ratio/fit.rscrates/qmrust-core/src/models/mt_ratio/mod.rscrates/qmrust-core/src/models/mt_ratio/model.rscrates/qmrust-core/src/models/mt_sat/config.rscrates/qmrust-core/src/models/mt_sat/fit.rscrates/qmrust-core/src/models/mt_sat/mod.rscrates/qmrust-core/src/models/mt_sat/model.rscrates/qmrust-core/src/mtsat_b1/calibrate.rscrates/qmrust-core/src/mtsat_b1/correct.rscrates/qmrust-core/src/mtsat_b1/fitvalues.rscrates/qmrust-core/src/mtsat_b1/lineshape.rscrates/qmrust-core/src/mtsat_b1/mat3.rscrates/qmrust-core/src/mtsat_b1/mod.rscrates/qmrust-core/src/mtsat_b1/pulse.rscrates/qmrust-core/src/mtsat_b1/rate.rscrates/qmrust-core/src/mtsat_b1/sim.rscrates/qmrust-core/src/mtsat_b1/surface.rscrates/qmrust-core/src/registry.rscrates/rust-bids/src/default_grouping.yamldocs/agents/ADDING-A-MODEL.mddocs/agents/ARCHITECTURE.mddocs/agents/DATA-PIPELINE.mddocs/getting-started.mdrecipes/bids/mt_ratio_config.yamlrecipes/bids/mt_sat_b1corr_config.yamlrecipes/bids/mt_sat_config.yamlrecipes/mtsat_b1_seq.yamlrecipes/non-bids/mt_ratio_config.yamlrecipes/non-bids/mt_sat_config.yaml
Comment on lines
+122
to
+143
| # MTR is a closed-form ratio (no fit, no unit conversion — both in percent), so | ||
| # agreement with qMRLab is exact to float rounding across the whole mask. | ||
| echo "Comparing mt_ratio MTR map to qMRLab FitResults..." | ||
| python3 ci/compare_maps.py \ | ||
| "$DATA/out_mtr/qmrust/sub-01/anat/sub-01_MTRmap.nii.gz" "$MTR_REF" \ | ||
| --rel-tol 0.001 --min-frac 0.99 --min-corr 0.999 --label mt_ratio-MTR | ||
|
|
||
| # mt_sat is closed-form (no B1 map in this dataset -> the uncorrected Helms | ||
| # path), so all three maps agree with qMRLab to double-precision round-off. | ||
| # MTSAT's near-zero (background-adjacent) voxels make a *relative* tolerance | ||
| # meaningless there, so its min-frac has margin while the correlation stays 1. | ||
| echo "Comparing mt_sat maps to qMRLab FitResults..." | ||
| python3 ci/compare_maps.py \ | ||
| "$DATA/out_mtsat/qmrust/sub-01/anat/sub-01_MTsat.nii.gz" "$MTSAT_REF_SAT" \ | ||
| --rel-tol 0.01 --min-frac 0.98 --min-corr 0.999 --label mt_sat-MTSAT | ||
| python3 ci/compare_maps.py \ | ||
| "$DATA/out_mtsat/qmrust/sub-01/anat/sub-01_T1map.nii.gz" "$MTSAT_REF_T1" \ | ||
| --rel-tol 0.01 --min-frac 0.99 --min-corr 0.999 --label mt_sat-T1 | ||
| python3 ci/compare_maps.py \ | ||
| "$DATA/out_mtsat/qmrust/sub-01/anat/sub-01_MTRmap.nii.gz" "$MTSAT_REF_MTR" \ | ||
| --rel-tol 0.001 --min-frac 0.99 --min-corr 0.999 --label mt_sat-MTR | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Describe the no-B1 comparison path accurately.
With no B1map, MTsat takes the no-correction path; the Helms factor is only used when a B1 map is present. Replace “uncorrected Helms path” with “uncorrected path” so the CI rationale matches the model contract.
Proposed comment fix
-# mt_sat is closed-form (no B1 map in this dataset -> the uncorrected Helms
-# path), so all three maps agree with qMRLab to double-precision round-off.
+# mt_sat is closed-form (no B1 map in this dataset -> the uncorrected path),
+# so all three maps agree with qMRLab to double-precision round-off.📝 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
| # MTR is a closed-form ratio (no fit, no unit conversion — both in percent), so | |
| # agreement with qMRLab is exact to float rounding across the whole mask. | |
| echo "Comparing mt_ratio MTR map to qMRLab FitResults..." | |
| python3 ci/compare_maps.py \ | |
| "$DATA/out_mtr/qmrust/sub-01/anat/sub-01_MTRmap.nii.gz" "$MTR_REF" \ | |
| --rel-tol 0.001 --min-frac 0.99 --min-corr 0.999 --label mt_ratio-MTR | |
| # mt_sat is closed-form (no B1 map in this dataset -> the uncorrected Helms | |
| # path), so all three maps agree with qMRLab to double-precision round-off. | |
| # MTSAT's near-zero (background-adjacent) voxels make a *relative* tolerance | |
| # meaningless there, so its min-frac has margin while the correlation stays 1. | |
| echo "Comparing mt_sat maps to qMRLab FitResults..." | |
| python3 ci/compare_maps.py \ | |
| "$DATA/out_mtsat/qmrust/sub-01/anat/sub-01_MTsat.nii.gz" "$MTSAT_REF_SAT" \ | |
| --rel-tol 0.01 --min-frac 0.98 --min-corr 0.999 --label mt_sat-MTSAT | |
| python3 ci/compare_maps.py \ | |
| "$DATA/out_mtsat/qmrust/sub-01/anat/sub-01_T1map.nii.gz" "$MTSAT_REF_T1" \ | |
| --rel-tol 0.01 --min-frac 0.99 --min-corr 0.999 --label mt_sat-T1 | |
| python3 ci/compare_maps.py \ | |
| "$DATA/out_mtsat/qmrust/sub-01/anat/sub-01_MTRmap.nii.gz" "$MTSAT_REF_MTR" \ | |
| --rel-tol 0.001 --min-frac 0.99 --min-corr 0.999 --label mt_sat-MTR | |
| # MTR is a closed-form ratio (no fit, no unit conversion — both in percent), so | |
| # agreement with qMRLab is exact to float rounding across the whole mask. | |
| echo "Comparing mt_ratio MTR map to qMRLab FitResults..." | |
| python3 ci/compare_maps.py \ | |
| "$DATA/out_mtr/qmrust/sub-01/anat/sub-01_MTRmap.nii.gz" "$MTR_REF" \ | |
| --rel-tol 0.001 --min-frac 0.99 --min-corr 0.999 --label mt_ratio-MTR | |
| # mt_sat is closed-form (no B1 map in this dataset -> the uncorrected path), | |
| # so all three maps agree with qMRLab to double-precision round-off. | |
| # MTSAT's near-zero (background-adjacent) voxels make a *relative* tolerance | |
| # meaningless there, so its min-frac has margin while the correlation stays 1. | |
| echo "Comparing mt_sat maps to qMRLab FitResults..." | |
| python3 ci/compare_maps.py \ | |
| "$DATA/out_mtsat/qmrust/sub-01/anat/sub-01_MTsat.nii.gz" "$MTSAT_REF_SAT" \ | |
| --rel-tol 0.01 --min-frac 0.98 --min-corr 0.999 --label mt_sat-MTSAT | |
| python3 ci/compare_maps.py \ | |
| "$DATA/out_mtsat/qmrust/sub-01/anat/sub-01_T1map.nii.gz" "$MTSAT_REF_T1" \ | |
| --rel-tol 0.01 --min-frac 0.99 --min-corr 0.999 --label mt_sat-T1 | |
| python3 ci/compare_maps.py \ | |
| "$DATA/out_mtsat/qmrust/sub-01/anat/sub-01_MTRmap.nii.gz" "$MTSAT_REF_MTR" \ | |
| --rel-tol 0.001 --min-frac 0.99 --min-corr 0.999 --label mt_sat-MTR |
🤖 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 `@ci/integration_osf.sh` around lines 122 - 143, Update the explanatory comment
above the mt_sat comparisons to describe the no-B1 case as the “uncorrected
path,” removing the inaccurate “uncorrected Helms path” wording while leaving
the comparison commands unchanged.
Comment on lines
+178
to
+197
| /// Read a `Named` model's per-role NIfTIs from `dir` (`<role>.nii.gz`), | ||
| /// stacked in the model's declared role order, preserving the first role's | ||
| /// spatial header. Only Named models have a role axis; a Series model has no | ||
| /// `<role>.nii.gz` layout to read from a directory. | ||
| fn read_named_nifti_source( | ||
| dir: &Path, | ||
| nii_mask: Option<&Path>, | ||
| model: &dyn Model, | ||
| ) -> Result<Source> { | ||
| let MeasurementKind::Named { roles } = model.measurement() else { | ||
| anyhow::bail!("--nii-dir is only for Named models (one <role>.nii.gz per role)"); | ||
| }; | ||
| let (data, header) = io::nifti::read_named_nii_volumes(dir, roles)?; | ||
| let mask = match nii_mask { | ||
| Some(p) => Some(io::nifti::read_mask_nifti(p)?), | ||
| None => None, | ||
| }; | ||
| Ok((data, mask, Vec::new(), Some(header))) | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
read_named_nifti_source drops the model's declared aux inputs — unlike read_mat_source.
read_mat_source (lines 148-161) dynamically loads every required_inputs() map from --mat-dir, but read_named_nifti_source returns Vec::new() for aux unconditionally (line 195). Any aux map (e.g. a B1map.nii.gz for mt_sat) sitting in --nii-dir is silently dropped from the bidsified output instead of being written to the derivatives tree.
🛠️ Proposed fix: mirror the mat-dir aux loading
fn read_named_nifti_source(
dir: &Path,
nii_mask: Option<&Path>,
model: &dyn Model,
) -> Result<Source> {
let MeasurementKind::Named { roles } = model.measurement() else {
anyhow::bail!("--nii-dir is only for Named models (one <role>.nii.gz per role)");
};
let (data, header) = io::nifti::read_named_nii_volumes(dir, roles)?;
let mask = match nii_mask {
Some(p) => Some(io::nifti::read_mask_nifti(p)?),
None => None,
};
- Ok((data, mask, Vec::new(), Some(header)))
+ let aux: Vec<(String, Array3<f64>)> = model
+ .required_inputs()
+ .into_iter()
+ .filter_map(|spec| {
+ let path = dir.join(format!("{}.nii.gz", spec.name));
+ path.exists()
+ .then(|| io::nifti::read_map_nifti(&path).map(|m| (spec.name.to_string(), m)))
+ })
+ .collect::<Result<Vec<_>>>()?;
+ Ok((data, mask, aux, Some(header)))
}📝 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
| /// Read a `Named` model's per-role NIfTIs from `dir` (`<role>.nii.gz`), | |
| /// stacked in the model's declared role order, preserving the first role's | |
| /// spatial header. Only Named models have a role axis; a Series model has no | |
| /// `<role>.nii.gz` layout to read from a directory. | |
| fn read_named_nifti_source( | |
| dir: &Path, | |
| nii_mask: Option<&Path>, | |
| model: &dyn Model, | |
| ) -> Result<Source> { | |
| let MeasurementKind::Named { roles } = model.measurement() else { | |
| anyhow::bail!("--nii-dir is only for Named models (one <role>.nii.gz per role)"); | |
| }; | |
| let (data, header) = io::nifti::read_named_nii_volumes(dir, roles)?; | |
| let mask = match nii_mask { | |
| Some(p) => Some(io::nifti::read_mask_nifti(p)?), | |
| None => None, | |
| }; | |
| Ok((data, mask, Vec::new(), Some(header))) | |
| } | |
| /// Read a `Named` model's per-role NIfTIs from `dir` (`<role>.nii.gz`), | |
| /// stacked in the model's declared role order, preserving the first role's | |
| /// spatial header. Only Named models have a role axis; a Series model has no | |
| /// `<role>.nii.gz` layout to read from a directory. | |
| fn read_named_nifti_source( | |
| dir: &Path, | |
| nii_mask: Option<&Path>, | |
| model: &dyn Model, | |
| ) -> Result<Source> { | |
| let MeasurementKind::Named { roles } = model.measurement() else { | |
| anyhow::bail!("--nii-dir is only for Named models (one <role>.nii.gz per role)"); | |
| }; | |
| let (data, header) = io::nifti::read_named_nii_volumes(dir, roles)?; | |
| let mask = match nii_mask { | |
| Some(p) => Some(io::nifti::read_mask_nifti(p)?), | |
| None => None, | |
| }; | |
| let aux: Vec<(String, Array3<f64>)> = model | |
| .required_inputs() | |
| .into_iter() | |
| .filter_map(|spec| { | |
| let path = dir.join(format!("{}.nii.gz", spec.name)); | |
| path.exists() | |
| .then(|| io::nifti::read_map_nifti(&path).map(|m| (spec.name.to_string(), m))) | |
| }) | |
| .collect::<Result<Vec<_>>>()?; | |
| Ok((data, mask, aux, Some(header))) | |
| } |
🤖 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-cli/src/bidsify.rs` around lines 178 - 197, Update
read_named_nifti_source to load every model.required_inputs() auxiliary map from
dir, mirroring read_mat_source, and return those loaded inputs instead of
Vec::new(). Preserve the existing role-volume, mask, and header handling while
ensuring aux files such as B1map.nii.gz are included in the resulting Source.
Comment on lines
+438
to
+441
| let (cfg, mut raw) = load_config_raw(&config_path)?; | ||
| if cfg.model == "mt_sat" { | ||
| inject_mt_sat_b1_correction(&mut raw)?; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Per-model if cfg.model == "mt_sat" checks bypass the Model trait dispatch surface.
Both run_fit and run_fit_bids special-case mt_sat by name before the registry is even consulted, which is the same anti-pattern the coding guideline for match cfg.model branches targets — just spelled as if instead of match. As more models need recipe-level config injection, this string-check pattern will keep growing in the CLI instead of going through the existing ModelEntry function-pointer dispatch (build/describe/dump).
As per coding guidelines, "Use the Model trait as the complete dispatch surface: do not add match cfg.model branches in the CLI, engine, simulation, or configuration code."
♻️ Suggested direction: route through `ModelEntry` instead of a name check
// registry.rs
pub struct ModelEntry {
// ...existing fields...
pub inject_extras: Option<fn(&mut serde_yaml::Value) -> anyhow::Result<()>>,
}Then in commands.rs, resolve entry first and call entry.inject_extras unconditionally instead of checking cfg.model:
- let (cfg, mut raw) = load_config_raw(&config_path)?;
- if cfg.model == "mt_sat" {
- inject_mt_sat_b1_correction(&mut raw)?;
- }
+ let (cfg, mut raw) = load_config_raw(&config_path)?;
+ let entry = qmrust_core::registry::by_name(&cfg.model).ok_or_else(|| anyhow::anyhow!("Unknown model: '{}'", cfg.model))?;
+ if let Some(inject) = entry.inject_extras {
+ inject(&mut raw)?;
+ }Also applies to: 696-699
🤖 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-cli/src/commands.rs` around lines 438 - 441, Replace the
per-model cfg.model == "mt_sat" checks in run_fit and run_fit_bids with
ModelEntry dispatch. Add an optional inject_extras function pointer to
ModelEntry, register inject_mt_sat_b1_correction there, resolve the entry before
recipe configuration, and invoke its injector when present so CLI code does not
branch on model names.
Source: Coding guidelines
Comment on lines
+52
to
+55
| let b1max = self.b1_ref * self.grid.b1_max_factor; | ||
| let b1s: Vec<f64> = (0..self.grid.b1_steps) | ||
| .map(|i| b1max * i as f64 / (self.grid.b1_steps as f64 - 1.0)) | ||
| .collect(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
grid.b1_steps == 1 divides by zero, producing NaN grid points silently.
b1max * i as f64 / (self.grid.b1_steps as f64 - 1.0) divides by zero when b1_steps == 1, yielding NaN for that grid point (and thus every (M0b, NaN, Raobs) sample) with no error — the corrupted values would flow straight into the fitted surface and the written calibration artifact.
🛡️ Proposed fix: validate before building the grid
pub fn run_mtsat_b1(args: MtSatB1Args) -> Result<()> {
let text =
std::fs::read_to_string(&args.seq).with_context(|| format!("reading {:?}", args.seq))?;
let cfg: SeqConfig = serde_yaml::from_str(&text)?;
+ anyhow::ensure!(
+ cfg.grid.b1_steps >= 2,
+ "grid.b1_steps must be >= 2 (got {})",
+ cfg.grid.b1_steps
+ );
// 1-2. Simulate the surface (parallel over the grid).🤖 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-cli/src/mtsat_b1.rs` around lines 52 - 55, Validate
self.grid.b1_steps before constructing b1s in the surrounding grid-building
method, requiring at least two steps and returning the established error type
for invalid configuration. Only perform the division in the existing map after
validation, preserving the current grid generation for valid step counts.
Comment on lines
+9
to
+14
| pub fn correction_factor(fv: &FitValues, b1_map: f64, raobs: f64) -> f64 { | ||
| let m0b = fv.m0b_vs_r1.m0b(raobs); | ||
| let ss_act = fv.ss_surface.eval(m0b, fv.b1_ref * b1_map, raobs); | ||
| let ss_nom = fv.ss_surface.eval(m0b, fv.b1_ref, raobs); | ||
| (ss_nom - ss_act) / ss_act | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect SsSurface::eval for any bounds/clamping on its return value.
fd -e rs surface.rs
ast-grep run --pattern 'fn eval($$$) { $$$ }' --lang rust crates/qmrust-core/src/mtsat_b1/surface.rsRepository: qMRLab/qmrust
Length of output: 194
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- outline: surface.rs ---'
ast-grep outline crates/qmrust-core/src/mtsat_b1/surface.rs --view expanded || true
echo
echo '--- relevant lines: surface.rs ---'
cat -n crates/qmrust-core/src/mtsat_b1/surface.rs | sed -n '1,260p'
echo
echo '--- correction_factor usage ---'
rg -n "correction_factor|ss_surface|struct SsSurface|impl .*SsSurface|eval\(" crates/qmrust-core/src/mtsat_b1 -nRepository: qMRLab/qmrust
Length of output: 8920
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- fitvalues.rs ---'
cat -n crates/qmrust-core/src/mtsat_b1/fitvalues.rs | sed -n '1,220p'
echo
echo '--- correct.rs ---'
cat -n crates/qmrust-core/src/mtsat_b1/correct.rs | sed -n '1,220p'
echo
echo '--- calibrate.rs ---'
cat -n crates/qmrust-core/src/mtsat_b1/calibrate.rs | sed -n '1,220p'
echo
echo '--- search for bounded/positive assumptions ---'
rg -n "bounded|positive|clamp|zero|division|correction_factor|mtsat" crates/qmrust-core/src/mtsat_b1 -nRepository: qMRLab/qmrust
Length of output: 11788
Guard the division in correction_factor. SsSurface::eval is an unconstrained polynomial, so ss_act can still hit zero or get very small; (ss_nom - ss_act) / ss_act would then produce Inf/NaN and poison the voxel’s MTSAT correction. Add a small fallback or epsilon check here.
🤖 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/src/mtsat_b1/correct.rs` around lines 9 - 14, Update
correction_factor to guard the denominator ss_act before computing (ss_nom -
ss_act) / ss_act; when ss_act is zero or sufficiently close to zero, return a
small safe fallback or use an epsilon denominator so the result never becomes
Inf or NaN.
Comment on lines
+56
to
+65
| // Ra from Raobs (MAMT_model_2007_5 lines 53-58). | ||
| let ra = { | ||
| let denom = p.rb - raobs + p.r; | ||
| let val = raobs - (p.r * m0b * (p.rb - raobs)) / denom; | ||
| if val.is_nan() { | ||
| 1.0 | ||
| } else { | ||
| val | ||
| } | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
ra fallback only guards against NaN, not ±inf.
If denom == 0 and the numerator is nonzero, val is ±inf, not NaN; the current check silently lets an infinite ra propagate into the rate matrix instead of triggering the fallback.
🛡️ Proposed fix
- if val.is_nan() {
+ if !val.is_finite() {
1.0
} else {
val
}📝 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
| // Ra from Raobs (MAMT_model_2007_5 lines 53-58). | |
| let ra = { | |
| let denom = p.rb - raobs + p.r; | |
| let val = raobs - (p.r * m0b * (p.rb - raobs)) / denom; | |
| if val.is_nan() { | |
| 1.0 | |
| } else { | |
| val | |
| } | |
| }; | |
| // Ra from Raobs (MAMT_model_2007_5 lines 53-58). | |
| let ra = { | |
| let denom = p.rb - raobs + p.r; | |
| let val = raobs - (p.r * m0b * (p.rb - raobs)) / denom; | |
| if !val.is_finite() { | |
| 1.0 | |
| } else { | |
| val | |
| } | |
| }; |
🤖 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/src/mtsat_b1/sim.rs` around lines 56 - 65, Update the `ra`
calculation’s fallback condition to reject all non-finite values, including
positive and negative infinity, rather than checking only `val.is_nan()`.
Preserve the existing `1.0` fallback for invalid results and the calculated
value for finite results.
added 9 commits
July 23, 2026 23:56
mamt_signal projected the steady-state Mza by the model's MTw flip (p.flip_angle) instead of the flip passed to that call (flip_deg). The VFA companion drives it at 5°/20°, so both VFA signals were mis-projected by sin(9°), corrupting R1app/Aapp and making the simulated MTsat surface under-saturate ~3-5×. This inflated the calibrated M0b-vs-R1 line above the physical 0-0.20 range. Now reads out with flip_deg (matching MAMT_model_2007_5, which sets Params.flipAngle to the VFA flip before each call). Verified against an independent Python re-implementation: surface MTsat at (M0b=0.1, b1=6.8, R1=0.83) 0.26% -> 1.38% (ref 1.40%); calibrated M0b(R1=0.83) 0.26 -> 0.072 (physical). Surfaced by an end-to-end run on real TardifLab data. Adds a vfa_apparent regression test (R1app~0.78, Aapp~1.02). Claude-Session: https://claude.ai/code/session_013BCm4Xwt1hFmr1ukQgV1JV
added 4 commits
July 24, 2026 13:18
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/qmrust-core/src/mtsat_b1/sim.rs (2)
279-327: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNothing in this layer pins a number against the ported reference. The tests are either relational (positive, bounded, monotone, converges) or recompute the implementation's own expression, so a systematic port error passes cleanly while the calibration surface drifts.
crates/qmrust-core/src/mtsat_b1/sim.rs#L279-L327: add a golden assertion onflash_signalandmtsat_simfortests_sample_params(), using values captured from the MATLAB reference.crates/qmrust-core/src/mtsat_b1/rate.rs#L136-L147: replace the re-derivedexpected_rrfbwith a hardcoded reference value, keeping the structuralrrfd == 0check.As per coding guidelines, "Preserve fitting behavior during refactors and validate output against fixtures; fitting output must not drift."
🤖 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/src/mtsat_b1/sim.rs` around lines 279 - 327, The tests in crates/qmrust-core/src/mtsat_b1/sim.rs:279-327 need golden assertions for flash_signal and mtsat_sim using tests_sample_params() and MATLAB-captured reference values; retain the existing relational checks. In crates/qmrust-core/src/mtsat_b1/rate.rs:136-147, replace the re-derived expected_rrfb with its hardcoded MATLAB reference value while preserving the structural rrfd == 0 assertion.Source: Coding guidelines
130-164: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winHoist the propagators out of the TR loop.
propagaterecomputesexpm5(a·t)andsolve5(a, b)on every call, but the(a, t)pairs are a fixed, small set: the ~pulse_stepssaturation matrices per offset atstep, anda_relaxatpulse_gap_dur/mt_grad_time/post_exc_relax. With the sample params the loop runs ~235 times, so ~99% of the matrix exponentials and solves are redundant — andmtsat_b1.rsrunsmtsat_sim(3 ×flash_signal) over the whole calibration grid.Precompute
(e, (e − I)·A⁻¹·B)per distinct(a, t)before the TR loop and reduce the inner step to a matvec plus an add; the result is bit-identical.🤖 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/src/mtsat_b1/sim.rs` around lines 130 - 164, Hoist propagation coefficient computation out of the TR loop in mtsat_sim: precompute expm5(a·t) and (e−I)·solve5(a,b) for each distinct saturation matrix/step pair and each a_relax duration (pulse_gap_dur, mt_grad_time, and post_exc_relax). Update propagate and the loop to reuse these precomputed coefficients, preserving the existing pulse selection and bit-identical matvec-plus-add behavior.
🤖 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/mtsat_b1/pulse.rs`:
- Around line 1-10: The sample_times grid and its documentation disagree for
non-integral dur/step ratios. Update sample_times to clamp the final generated
sample to dur so the returned grid includes the pulse endpoint, and adjust the
sampling logic as needed to avoid overshooting; preserve the existing behavior
for exact ratios and ensure the endpoint is used by downstream Hann-window and
RMS calculations.
- Around line 29-31: Guard the RMS normalization in the pulse-shape construction
around mean_sq and scale: detect when mean_sq is zero before taking its square
root or dividing by it, and surface the invalid pulse configuration explicitly
instead of returning inf/NaN amplitudes. Preserve the existing scaling behavior
for positive mean_sq values.
In `@crates/qmrust-core/src/mtsat_b1/sim.rs`:
- Around line 99-114: Unify the saturation grid and propagation duration so
integration equals pulse_dur without changing fitting results. In
crates/qmrust-core/src/mtsat_b1/sim.rs:99-114, derive pulse_steps from the grid
returned by gausshann_omega and explicitly account for any trailing remainder
instead of independently ceiling pulse_dur / step. In
crates/qmrust-core/src/mtsat_b1/pulse.rs:1-10, make gausshann_omega cover the
documented [0, pulse_dur] range by clamping its final sample to dur, or
consistently preserve truncated-grid semantics and have sim.rs honor them;
validate output against existing fixtures.
---
Nitpick comments:
In `@crates/qmrust-core/src/mtsat_b1/sim.rs`:
- Around line 279-327: The tests in
crates/qmrust-core/src/mtsat_b1/sim.rs:279-327 need golden assertions for
flash_signal and mtsat_sim using tests_sample_params() and MATLAB-captured
reference values; retain the existing relational checks. In
crates/qmrust-core/src/mtsat_b1/rate.rs:136-147, replace the re-derived
expected_rrfb with its hardcoded MATLAB reference value while preserving the
structural rrfd == 0 assertion.
- Around line 130-164: Hoist propagation coefficient computation out of the TR
loop in mtsat_sim: precompute expm5(a·t) and (e−I)·solve5(a,b) for each distinct
saturation matrix/step pair and each a_relax duration (pulse_gap_dur,
mt_grad_time, and post_exc_relax). Update propagate and the loop to reuse these
precomputed coefficients, preserving the existing pulse selection and
bit-identical matvec-plus-add behavior.
🪄 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: f5abf9b3-fd46-4dff-b9e4-45e88ee814c9
📒 Files selected for processing (10)
crates/qmrust-cli/src/commands.rscrates/qmrust-cli/src/mtsat_b1.rscrates/qmrust-core/src/mtsat_b1/fitvalues.rscrates/qmrust-core/src/mtsat_b1/mat5.rscrates/qmrust-core/src/mtsat_b1/mod.rscrates/qmrust-core/src/mtsat_b1/pulse.rscrates/qmrust-core/src/mtsat_b1/rate.rscrates/qmrust-core/src/mtsat_b1/sim.rsdocs/agents/ARCHITECTURE.mdrecipes/mtsat_b1_seq.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- recipes/mtsat_b1_seq.yaml
- docs/agents/ARCHITECTURE.md
- crates/qmrust-core/src/mtsat_b1/fitvalues.rs
- crates/qmrust-cli/src/mtsat_b1.rs
- crates/qmrust-cli/src/commands.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/qmrust-cli/src/commands.rs (2)
1552-1578: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftAdd fixture-backed regressions for named ordering and MTsat injection.
The named-collection test covers only failure cases, while the MTsat tests assert only that a synthetic result changes. No test proves the role-ordered data/protocol stack or final MTsat output against a known fixture, and the injection tests bypass
run_fit/run_fit_bidswiring. Add at least one shuffled named-volume fixture with expected fitting output through the production path.As per coding guidelines, “Preserve fitting behavior during refactors and validate output against fixtures; fitting output must not drift.”
Also applies to: 1997-2020, 2023-2046, 2049-2065
🤖 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-cli/src/commands.rs` around lines 1552 - 1578, Add fixture-backed regression tests covering shuffled named-volume ordering and MTsat injection through the production run_fit/run_fit_bids paths, rather than testing only synthetic changes or load_collection failures. Validate the role-ordered data/protocol stack and final MTsat fitting output against known expected fixture results, including correct named-role mapping and stable fitting behavior.Source: Coding guidelines
79-100: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winResolve
fitvaluesrelative to the config file.
recipes/bids/mt_sat_b1corr_config.yamlusesfitvalues: fitvalues.yaml, but this helper reads that path against the process CWD. Pass the config directory into the helper and add a relative-path test.🤖 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-cli/src/commands.rs` around lines 79 - 100, Update inject_mt_sat_b1_correction to accept the recipe config directory and resolve relative fitvalues paths against it before reading, while preserving absolute paths. Update its caller to pass the config file’s parent directory, and add a test covering a relative fitvalues path.
🧹 Nitpick comments (1)
crates/qmrust-cli/src/commands.rs (1)
2024-2027: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winState the current
b1_refauthority rule.Replace the historical “no longer parsed” wording with the invariant: the serialized fitvalues artifact is authoritative, and any recipe-level
b1_refis ignored.Suggested wording
- // A `b1_ref` in the recipe's `b1_correction` block is no longer - // parsed — it is ignored (not an error, not a warning). The - // artifact's own `b1_ref` (6.8 here) is what's inlined and used. + // The fitvalues artifact is authoritative for `b1_ref`; any + // recipe-level `b1_ref` is intentionally ignored.🤖 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-cli/src/commands.rs` around lines 2024 - 2027, Update the comment in mt_sat_recipe_stray_b1_ref_is_ignored to state that the serialized fitvalues artifact is authoritative for b1_ref, and any recipe-level b1_ref is ignored. Remove the historical wording about it being “no longer parsed,” while preserving the existing example context.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@crates/qmrust-cli/src/commands.rs`:
- Around line 1552-1578: Add fixture-backed regression tests covering shuffled
named-volume ordering and MTsat injection through the production
run_fit/run_fit_bids paths, rather than testing only synthetic changes or
load_collection failures. Validate the role-ordered data/protocol stack and
final MTsat fitting output against known expected fixture results, including
correct named-role mapping and stable fitting behavior.
- Around line 79-100: Update inject_mt_sat_b1_correction to accept the recipe
config directory and resolve relative fitvalues paths against it before reading,
while preserving absolute paths. Update its caller to pass the config file’s
parent directory, and add a test covering a relative fitvalues path.
---
Nitpick comments:
In `@crates/qmrust-cli/src/commands.rs`:
- Around line 2024-2027: Update the comment in
mt_sat_recipe_stray_b1_ref_is_ignored to state that the serialized fitvalues
artifact is authoritative for b1_ref, and any recipe-level b1_ref is ignored.
Remove the historical wording about it being “no longer parsed,” while
preserving the existing example context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6a3e36c3-2e11-49ea-ae09-bee028de3206
📒 Files selected for processing (5)
crates/qmrust-cli/src/commands.rscrates/qmrust-core/src/models/mt_sat/config.rscrates/qmrust-core/src/models/mt_sat/fit.rsdocs/agents/DATA-PIPELINE.mdrecipes/bids/mt_sat_b1corr_config.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- recipes/bids/mt_sat_b1corr_config.yaml
- docs/agents/DATA-PIPELINE.md
- crates/qmrust-core/src/models/mt_sat/fit.rs
- crates/qmrust-core/src/models/mt_sat/config.rs
added 9 commits
July 25, 2026 09:16
…rame; Instance cues
…os (rule → why → Instance)
…overshoots, matches doc/reference
This was referenced Jul 29, 2026
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.
Summary by CodeRabbit
MTRexport.mtsat-b1command to generate MTsat B1-correction artifacts.bidsifyand fitting for named, per-role NIfTI inputs via--nii-dir, with correct role-aware ordering for named collections.