Port vfa_t1 from qMRLab - #13
Merged
Merged
Conversation
added 8 commits
July 31, 2026 11:08
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds a variable flip angle T1 model with linear and nonlinear fitting, B1 correction, BIDS integration, OSF validation, example-dataset generation, playground data, and model documentation. ChangesVFA T1 model and fitting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Bidsify
participant VfaT1Model
participant VfaT1Fitter
participant Playground
User->>Bidsify: provide VFA volumes, mask, and B1 map
Bidsify->>VfaT1Model: create BIDS dataset
VfaT1Model->>VfaT1Fitter: assemble flip-angle samples
VfaT1Fitter->>Playground: return M0 and T1 outputs
Playground->>User: display maps and probe results
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
agahkarakuzu
marked this pull request as ready for review
August 1, 2026 04:13
agahkarakuzu
marked this pull request as draft
August 1, 2026 04:25
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
crates/qmrust-core/src/models/vfa_t1/fit.rs (2)
64-73: 🚀 Performance & Scalability | 🔵 TrivialConsider a shared helper for the SPGR signal equation.
forward()andVfaProblem::residual_for()both computem0 * a.sin() * (1.0 - e) / (1.0 - e * a.cos())independently. Extracting a single function for the closed-form signal would remove the duplication and prevent the two from drifting if the equation changes.Also applies to: 152-164
🤖 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/models/vfa_t1/fit.rs` around lines 64 - 73, Extract the duplicated SPGR signal equation from VfaT1::forward and VfaProblem::residual_for into one shared helper, then update both callers to use it with the same m0, angle, and relaxation-factor inputs. Preserve the existing numerical behavior while ensuring future equation changes are made in only the helper.
89-108: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDocument intentional qMRLab parity for
slope >= 1.0.Compute_M0_T1_OnSPGRuses the same guard and permits this case, which can produce a finite non-positiveT1. Update thefit_linearcomment to document this behavior instead of suggesting that every non-physical solve returnsNaN.🤖 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/models/vfa_t1/fit.rs` around lines 89 - 108, Update the documentation comment for fit_linear to state that qMRLab parity permits slope >= 1.0 after the existing guard, which may produce a finite non-positive T1; do not change the implementation or imply that every non-physical solve returns NaN.crates/rust-bids/src/vocab.rs (1)
216-238: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider renaming
custom_suffixesto reflect its actual contents.
is_custom_suffixandis_declared_suffixboth read the same underlyingcustom_suffixesset, but now answer different questions ("is this non-canonical" vs. "is this declared at all"). The field holds canonical and non-canonical suffixes alike sincebids()inserts every registered model's suffix into it. The namecustom_suffixesno longer reflects that, which is why two long doc comments are needed to keep the two predicates straight.Rename the field (e.g. to
declared_suffixes) so its name matchesis_declared_suffix's semantics directly, andis_custom_suffixreads as the derived, narrower predicate.🤖 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/rust-bids/src/vocab.rs` around lines 216 - 238, Rename the underlying custom_suffixes field to declared_suffixes throughout its definition, initialization, and all references, including bids(), is_custom_suffix(), and is_declared_suffix(). Preserve the existing set contents and predicate behavior, with is_custom_suffix remaining the canonical-excluding derived check.
🤖 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 @.claude/skills/porting-qmrlab-models/SKILL.md:
- Around line 69-81: Add the promised executable identity check in the
Series-model validation example: after constructing ids with
engine::build_volume_ids, compare each VolumeId::Params entry against the
corresponding m.forward(..).series()[i].params using assert_eq! or an equivalent
loop. Keep the comparison aligned by volume index so any drift between
protocol-derived identities and forward sample parameters fails immediately.
In `@crates/qmrust-core/src/models/vfa_t1/config.rs`:
- Line 54: Update the flip-angle validation in the configuration validation
method to reject non-finite values, including NaN, before sorting. Ensure
invalid angles produce the existing validation error path so sort_by’s
partial_cmp unwrap in the same method cannot receive NaN.
- Around line 47-67: Update validate_protocol to reject duplicate flip angles
after validating and sorting self.flip_angles, preserving the existing
acceptance of distinct angles and returning a clear validation error when any
adjacent sorted values are equal. Ensure this uniqueness check runs before
returning Ok so fit() never receives duplicate expected angles.
In `@scripts/docsfig/dataset.py`:
- Around line 141-143: Update the auxiliary matching logic around found and
aux[spec["name"]] so ambiguous matches cannot be silently accepted: require
exactly one matching derivative before assignment, and fail clearly when
multiple matches are found. Preserve the existing assignment for a unique match
and the current behavior when no match exists.
---
Nitpick comments:
In `@crates/qmrust-core/src/models/vfa_t1/fit.rs`:
- Around line 64-73: Extract the duplicated SPGR signal equation from
VfaT1::forward and VfaProblem::residual_for into one shared helper, then update
both callers to use it with the same m0, angle, and relaxation-factor inputs.
Preserve the existing numerical behavior while ensuring future equation changes
are made in only the helper.
- Around line 89-108: Update the documentation comment for fit_linear to state
that qMRLab parity permits slope >= 1.0 after the existing guard, which may
produce a finite non-positive T1; do not change the implementation or imply that
every non-physical solve returns NaN.
In `@crates/rust-bids/src/vocab.rs`:
- Around line 216-238: Rename the underlying custom_suffixes field to
declared_suffixes throughout its definition, initialization, and all references,
including bids(), is_custom_suffix(), and is_declared_suffix(). Preserve the
existing set contents and predicate behavior, with is_custom_suffix remaining
the canonical-excluding derived check.
🪄 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: f7351284-4ce3-4548-88da-426fbc356d40
⛔ Files ignored due to path filters (15)
docs/playground/data/inversion_recovery.nii.gzis excluded by!**/*.gzdocs/playground/data/inversion_recovery_mask.nii.gzis excluded by!**/*.gzdocs/playground/data/mono_t2.nii.gzis excluded by!**/*.gzdocs/playground/data/mono_t2_mask.nii.gzis excluded by!**/*.gzdocs/playground/data/mt_ratio.nii.gzis excluded by!**/*.gzdocs/playground/data/mt_ratio_mask.nii.gzis excluded by!**/*.gzdocs/playground/data/mt_sat.nii.gzis excluded by!**/*.gzdocs/playground/data/qmt_spgr.nii.gzis excluded by!**/*.gzdocs/playground/data/qmt_spgr_B0map.nii.gzis excluded by!**/*.gzdocs/playground/data/qmt_spgr_B1map.nii.gzis excluded by!**/*.gzdocs/playground/data/qmt_spgr_R1map.nii.gzis excluded by!**/*.gzdocs/playground/data/qmt_spgr_mask.nii.gzis excluded by!**/*.gzdocs/playground/data/vfa_t1.nii.gzis excluded by!**/*.gzdocs/playground/data/vfa_t1_B1map.nii.gzis excluded by!**/*.gzdocs/playground/data/vfa_t1_mask.nii.gzis excluded by!**/*.gz
📒 Files selected for processing (33)
.claude/skills/porting-qmrlab-models/SKILL.mdci/integration_osf.shcrates/qmrust-cli/src/bidsify.rscrates/qmrust-core/src/models/mod.rscrates/qmrust-core/src/models/vfa_t1/config.rscrates/qmrust-core/src/models/vfa_t1/fit.rscrates/qmrust-core/src/models/vfa_t1/mod.rscrates/qmrust-core/src/models/vfa_t1/model.rscrates/qmrust-core/src/registry.rscrates/rust-bids/src/default_grouping.yamlcrates/rust-bids/src/table.rscrates/rust-bids/src/vocab.rsdocs/figures/qmt_spgr/aux.webpdocs/figures/qmt_spgr/curve.webpdocs/figures/vfa_t1/aux.webpdocs/figures/vfa_t1/curve.webpdocs/figures/vfa_t1/inputs.webpdocs/figures/vfa_t1/outputs.webpdocs/index.mddocs/models/index.mddocs/models/relaxometry/vfa_t1.mddocs/playground/data/index.jsondocs/playground/data/qmt_spgr.jsondocs/playground/data/sources.jsondocs/playground/data/vfa_t1.jsondocs/playground/inputs.jsdocs/playground/model.jsdocs/references.bibrecipes/bids/vfa_t1_config.yamlrecipes/non-bids/vfa_t1_config.yamlscripts/docsfig/dataset.pyscripts/make_bids_examples.shscripts/tests/test_dataset.py
agahkarakuzu
marked this pull request as ready for review
August 1, 2026 04:27
added 2 commits
August 1, 2026 00:58
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.
Ports qMRLab's
vfa_t1(variable flip angle T1) into qmrust, plus the fixes the port surfaced.The model
Fram et al. (1987) linearized SPGR fit. Dividing the steady-state signal by
sin(α·B1)andtan(α·B1)linearizes it, so T1 and M0 come from an ordinary least-squares line — closed form, no iteration. A transmit map scales the nominal flip angles when present.VFA, per-volumeflipentity,FlipAngleper volume andRepetitionTimeExcitationglobal.B1map←TB1map.T1map(s) andM0map.sequential_set: { by: [flip] }grouping; one registry line; recipes for both the BIDS and non-BIDS paths.Validation against qMRLab
Fitted qMRLab's own OSF dataset (
osf.io/7wcvh, FA = [3°, 20°], TR = 15 ms, with the shipped B1 map) and compared voxelwise to itsFitResults:Both maps agree to the float32 precision they are stored in. Wired into
ci/integration_osf.shalongside the existing model comparisons.Fixes this surfaced
.bidsignoreno longer hides a model's own volumes.is_custom_suffixreturned true for every registered suffix, including canonical ones, sobidsifywrote*IRT1*/*MESE*/*MTR*/*MTS*lines into datasets it produced. Two separate questions were conflated:.bidsignoreline?QMTSPGR).bidsignorehide this file from qmrust?is_declared_suffixnow answers the read question andis_custom_suffixthe write one.bidsifyalso removes a stale line, so roots written before the fix self-heal. Verified against all six published archives, every one of which still carries a stale line:Aux maps filed under
fmap/were invisible to the docs/playground generator, which globbed onlyanat/.qmt_spgrhad been shipping its playground payload withoutB1mapandB0map— the app fitted uncorrected while the CLI corrected, with no symptom to point at.The playground landed on the file list after a successful dataset load instead of the viewer.
Dataset hosting
Adding
ds-vfa.zipto Zenodo minted a new version, sodocs/playground/data/sources.jsonmoves to record21696048. All six archives were re-verified at the new record: HTTP 200 withAccess-Control-Allow-Origin: *. The concept id can't replace this — it 404s on the/api/files route, so publishing a dataset will always mean touching this file.Also
scripts/make_bids_examples.shbuildsds-vfa;ci/integration_osf.shfetches, bidsifies, fits and compares it.Seriesvolume-identity trap (see PR 2 for what that one cost).Gates
cargo test --workspace,cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings, bothwasm32-unknown-unknownbuilds,node --test scripts/tests/*.test.mjs,python3 -m unittest discover -s scripts/tests.Summary by CodeRabbit