Port b1_dam and b1_afi, and group models by method family - #28
Merged
Conversation
added 7 commits
August 2, 2026 00:39
…honest geometry --nii-data is repeatable, so a Series model whose dataset ships one 3D file per volume reads without being restacked first; read_named_nii_volumes now delegates to a shared stack_nii_volumes. datatype_for_suffix is the single fmap/anat rule, replacing a helper that knew only *B1map/*B0map, and it drives all three writers: the raw acquisition, the preprocessed aux maps and the derivative outputs. A model that wrote TB1map into anat/ would be invisible to the B1 resolver that reads fmap/. A source header declaring neither qform nor sform defines no geometry, so it is replaced with a synthesized minimal one rather than propagated. qMRLab's b1_dam example declares a 2 micrometre slice, which renders as an empty sliver; the voxel data is untouched either way.
An unmatched mask: block resolved silently, and the recipe is echoed verbatim into the provenance Parameters, so the sidecar of an unmasked fit read exactly like a masked one; only Sources distinguished them, by omission. A blank entity is now unconstrained rather than a match on the empty string. Recipes for datasets that ship no mask can then leave desc empty instead of naming one the data does not have, and still pick up whatever mask a reader's own dataset carries.
Two closed-form B1+ mapping models, both validated voxelwise against qMRLab's own FitResults on its OSF example data: max relative difference 6e-16, with identical NaN footprints. Reproducing MATLAB's arithmetic exactly is what makes those footprints match. b1_dam takes the magnitude of a complex arccos where the signal ratio leaves [-1, 1], which 57 of 4096 voxels do. b1_afi pins an unphysical ratio to zero via cos_arg*(r<=1) + 1*(r>1), whose IEEE behaviour is load-bearing: Inf*0 and NaN*0 are NaN, so a non-finite ratio stays NaN instead of being pinned. An if/else clamp would silently move 103484 voxels. Both recover an amplitude alongside B1 so forward reproduces the measured signal rather than a unit-scaled shape of it; it is a diagnostic output, not a written map. b1_afi's forward is qMRLab's own steady-state afi_equation, which depends on a T1 the closed form never recovers, so T1 is a fixed parameter and recovery carries the method's real TR<<T1 bias rather than being exact. Entities follow the BIDS schema rather than convention: TB1DAM requires flip, TB1AFI is an RFFieldMaps suffix indexed by acq-tr1/acq-tr2. Field mapping joins the registry taxonomy as its own category.
…anat/ Both new models get the full chain in integration_osf.sh: fetch, bidsify, a BIDS-path fit and a voxelwise compare_maps against qMRLab's FitResults, plus a ds-tb1dam/ds-tb1afi block in make_bids_examples.sh. Three places globbed only anat/ and so silently dropped a dataset whose files are field maps: assert_maps, and the raw-acquisition and output-map globs in docsfig. The aux glob already carried a comment about this exact hazard; the others now match it. The zip step prunes .DS_Store. The playground's own resolver reports files it cannot account for, so one shows up as 'not a recognized BIDS file' in front of a reader. mt_sat's recipes leave the mask desc blank: their example dataset has none.
Relaxometry splits into T1 and T2, and magnetization transfer subdivides into semi-quantitative and quantitative. The gallery derives the whole tree from Category::order by grouping consecutive runs, so neither the order nor the membership is restated in the generator. Only the family reaches the URL. Both MT categories share the magnetization-transfer directory, so those pages do not move; the three relaxometry pages do, and their old copies are removed rather than left as duplicates.
The picker is a collapsible two-level tree over the same registry taxonomy the documentation gallery uses: each payload carries family, subgroup, icon and order, so adding a model or re-cutting the categories touches the registry and nothing in the app. The <select> stays the value and the tree mirrors it, so a dropped dataset selecting a model still works and the control degrades to a native select. Options are built in taxonomy order, which is also what decides the model loaded on arrival. Its background is opaque rather than the shared panel token: it hangs over the recipe form, and every theme's --panel is translucent. growMask shrinks or grows a computed mask, applied from the pristine segmentation each time rather than compounding, since morphology does not undo itself. Axes of extent 1 are skipped: four of five example datasets are one slice thick, and counting the absent z would erase the mask on the first shrink. Segment's classical method is named for what it does, and the protocol slider says edit/lock rather than override. The wasm resolver forwards input-resolution warnings through the same channel as grouping warnings.
Adds the two field-map models' figures and slices, and re-derives index.json. Every payload gains the taxonomy the picker groups by and its BIDS suffix; the existing .nii.gz slices are byte-level rewrites from the same regeneration, same content. sources.json moves to Zenodo record 21753826, which is where the two new archives live. ds-tb1dam had to be replaced there: its first upload carried .DS_Store files and the unusable source geometry.
📝 WalkthroughWalkthroughThis PR adds B1 AFI and B1 DAM models, repeated 3D NIfTI input support, suffix-based BIDS output routing, expanded model taxonomy metadata, mask warnings, integration workflows, documentation, and playground model-picker and mask-morphology controls. ChangesB1 Models and BIDS Infrastructure
Playground and Documentation
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as bidsify CLI
participant NiftiIO as io/nifti.rs
participant Model as B1AfiModel/B1DamModel
participant BIDS as rust-bids
participant FS as BIDS output
CLI->>NiftiIO: stack_nii_volumes(nii_data paths)
NiftiIO-->>CLI: stacked volumes and header
CLI->>Model: validate protocol and volume count
Model-->>CLI: BIDS suffix and output metadata
CLI->>BIDS: datatype_for_suffix(suffix)
BIDS-->>CLI: fmap or anat
CLI->>FS: write acquisition volumes and derivatives
sequenceDiagram
participant Fit as fit command
participant Inputs as rust-bids inputs
participant WASM as resolve_bids
participant Console as CLI output
Fit->>Inputs: resolve configured mask
Inputs-->>Inputs: record missing-mask warning
Inputs-->>Fit: continue without mask
Fit->>Console: print resolver warning
WASM->>Inputs: resolve BIDS inputs
Inputs-->>WASM: grouping and input warnings
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
The subset gained circle-dot, spline-pointer (plus a mirrored variant), waves-arrow-up and list-collapse, and lost swatch-book once it was unused. The version is unchanged: all four are byte-identical at the pinned lucide 1.27.0, so only the digest moves.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
crates/rust-bids/src/inputs.rs (1)
309-380: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the task-specific model reference.
Line 367 ties this general resolution test to
b1_dam/b1_afi. Describe only the no-mask dataset condition.As per coding guidelines, comments “must not describe history, rejected alternatives, review context, or task references.”
🤖 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/inputs.rs` around lines 309 - 380, Remove the task-specific “b1_dam/b1_afi” reference from the comment in test a_mask_the_dataset_does_not_hold_is_reported_not_silently_skipped, replacing it with a generic description of a dataset containing no mask. Preserve the test logic and all other explanatory comments.Source: Coding guidelines
scripts/gen_model_docs.py (1)
430-433: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the taxonomy comments and docstring.
render_gallerynow groups cards by family and optional subgroup. The docstring on Line 423 still says “grouped by category”. The comment on Line 433 says “renders exactly as before”. Replace both with the current family, subgroup, and heading-level contract. Remove the historical comparison.As per coding guidelines, stale terminology must be removed and documentation must describe the current contract.
🤖 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/gen_model_docs.py` around lines 430 - 433, Update the render_gallery docstring and adjacent taxonomy comment to describe the current two-level family/subgroup grouping and corresponding heading-level contract, replacing the stale “grouped by category” terminology. Remove the historical “renders exactly as before” comparison while retaining the explanation that registry ordering and consecutive grouping rebuild the taxonomy.Source: Coding guidelines
docs/playground/picker.js (1)
57-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated taxonomy sort comparator across
app.jsandpicker.js. Both files independently implement the same(category_order ?? 0)thentitle.localeComparecomparator to order models by taxonomy; this is a duplicated source of truth that can silently drift if the ordering rule ever changes in only one place.
docs/playground/picker.js#L57-L69: export the comparator (e.g.export function compareByTaxonomy(a, b) { ... }) frompicker.jsfor bothapp.jsandbuildModelTreeto reuse; sinceentrieshere is already derived from an already-sorted<select>, this local re-sort can then be dropped entirely.docs/playground/app.js#L377-L407: import and use the same shared comparator frompicker.jsinstead of re-implementing it inline when buildingordered.🤖 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/picker.js` around lines 57 - 69, Export a shared compareByTaxonomy comparator from docs/playground/picker.js and remove the redundant sort in buildModelTree, since its entries come from an already-sorted select. In docs/playground/app.js, import and reuse compareByTaxonomy when constructing ordered instead of maintaining an inline comparator.crates/qmrust-cli/src/commands.rs (1)
845-857: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd subject/session context to
load_aux_and_maskwarnings.
load_aux_and_maskprints each resolver warning as"warning: {w}"with no collection identity. The existing collection-warning loop a few lines above prints" warning ({}): {}", c.subject, w.message. Whenrun_fit_bidsprocesses multiple subjects, an unmatched-mask warning here (for example, "fitting the whole image unmasked") gives no way to tell which subject or session triggered it. Pass the collection's subject/session (or theidentitymap already available to this function) into the message.♻️ Proposed fix
fn load_aux_and_mask( table: &[rust_bids::BidsRow], model: &dyn Model, identity: &std::collections::BTreeMap<String, String>, mask_spec: Option<&rust_bids::MaskSpec>, bids_dir: &Path, ) -> Result<(AuxMaps, Option<Array3<bool>>, Vec<String>)> { let paths = rust_bids::resolve_input_paths(table, model, identity, mask_spec)?; - for w in &paths.warnings { - eprintln!("warning: {w}"); - } + let label = identity.get("sub").map(String::as_str).unwrap_or("?"); + for w in &paths.warnings { + eprintln!(" warning ({label}): {w}"); + }🤖 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 845 - 857, Update the warning loop in load_aux_and_mask to include the relevant subject/session context from the available identity map, while preserving each resolver warning’s message and warning prefix. Match the existing collection-warning formatting where practical so warnings from multiple run_fit_bids inputs can be distinguished.crates/qmrust-cli/src/io/nifti.rs (1)
161-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit tests for the new multi-volume NIfTI ingestion path. Both sites implement or use the new multi-file stacking logic, and neither has a unit test that runs under a plain
cargo test; the only coverage today is the#[ignore]d OSF integration script.
crates/qmrust-cli/src/io/nifti.rs#L161-L196: add a test forstack_nii_volumescovering the dims-mismatch bail message and that output volumeicorresponds topaths[i](order preservation), plus that the first file's header is the one kept.crates/qmrust-cli/src/bidsify.rs#L208-L241: add a test forread_nifti_source'smanybranch asserting the count-mismatch error message whenpaths.len() != model.n_volumes().🤖 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/nifti.rs` around lines 161 - 196, Add plain cargo test unit coverage for the multi-volume ingestion paths: in crates/qmrust-cli/src/io/nifti.rs lines 161-196, test stack_nii_volumes for the dims-mismatch error, paths[i] order preservation, and retention of the first volume’s header; in crates/qmrust-cli/src/bidsify.rs lines 208-241, test read_nifti_source’s many branch for the paths.len() versus model.n_volumes() count-mismatch error.
🤖 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/models/b1_afi/config.rs`:
- Around line 37-58: Reject non-finite protocol values in both validation sites:
in crates/qmrust-core/src/models/b1_afi/config.rs lines 37-58, update the
repetition-time and flip-angle guards to use positive validation that also
rejects NaN; in crates/qmrust-core/src/models/b1_dam/config.rs lines 42-50,
update the alpha guard similarly and additionally reject non-finite alpha2
values.
In `@crates/qmrust-core/src/models/b1_afi/fit.rs`:
- Around line 75-82: Handle incomplete B1 AFI protocols before any direct
repetition_times indexing: update the B1 AFI flow around B1AfiFitter::tr1 and
B1AfiFitter::tr2, plus bids_volume, forward, fit, and fit_voxel, to reject or
safely short-circuit one-time and empty protocols before accessing missing
entries, including before writing them. Do not rely solely on NaN fallbacks in
tr1/tr2; preserve valid two-timepoint behavior. The sibling site
crates/qmrust-core/src/models/b1_dam/fit.rs:57-59 requires no direct change
because B1DamFitter::alpha has no equivalent described-model path.
In `@crates/rust-bids/src/inputs.rs`:
- Around line 172-179: Update the None fallback in the describe_mask match
within the warnings construction to state that no mask matching the requested
suffix was found, rather than claiming the dataset contains no masks. Include
spec.suffix in the warning so unmatched suffix requests are accurately
identified.
In `@crates/rust-bids/src/vocab.rs`:
- Around line 246-264: Update datatype_for_suffix to classify the canonical
field-map suffixes epi, phasediff, phase1, phase2, magnitude, magnitude1, and
magnitude2 as fmap alongside the existing field-map cases, and add regression
tests covering them. If the function is intentionally qMRI-only instead, clearly
narrow its documented contract and ensure generic BIDS suffixes cannot reach it.
In `@docs/models/field-mapping/b1_afi.md`:
- Around line 108-114: The BIDS derivative examples use the incorrect datatype
directory. Update the output path in docs/models/field-mapping/b1_afi.md lines
108-114 and docs/models/field-mapping/b1_dam.md lines 112-118 from anat to fmap,
leaving the filenames and surrounding documentation unchanged.
In `@docs/models/field-mapping/b1_dam.md`:
- Around line 68-74: Update the “Diagnostic outputs” documentation near the
B1DamModel::bids_outputs reference so it does not claim that A is written
alongside quantitative maps. Describe A as a diagnostic fit output, or
explicitly document its non-BIDS behavior separately, while preserving the
distinction that it is not a quantitative measurement.
In `@docs/playground/picker.js`:
- Around line 152-222: In renderTree, assign role="group" to both the
model-family-body container and each model-subgroup-body container before
appending their role="treeitem" rows, preserving the existing hierarchy and
rendering behavior.
---
Nitpick comments:
In `@crates/qmrust-cli/src/commands.rs`:
- Around line 845-857: Update the warning loop in load_aux_and_mask to include
the relevant subject/session context from the available identity map, while
preserving each resolver warning’s message and warning prefix. Match the
existing collection-warning formatting where practical so warnings from multiple
run_fit_bids inputs can be distinguished.
In `@crates/qmrust-cli/src/io/nifti.rs`:
- Around line 161-196: Add plain cargo test unit coverage for the multi-volume
ingestion paths: in crates/qmrust-cli/src/io/nifti.rs lines 161-196, test
stack_nii_volumes for the dims-mismatch error, paths[i] order preservation, and
retention of the first volume’s header; in crates/qmrust-cli/src/bidsify.rs
lines 208-241, test read_nifti_source’s many branch for the paths.len() versus
model.n_volumes() count-mismatch error.
In `@crates/rust-bids/src/inputs.rs`:
- Around line 309-380: Remove the task-specific “b1_dam/b1_afi” reference from
the comment in test
a_mask_the_dataset_does_not_hold_is_reported_not_silently_skipped, replacing it
with a generic description of a dataset containing no mask. Preserve the test
logic and all other explanatory comments.
In `@docs/playground/picker.js`:
- Around line 57-69: Export a shared compareByTaxonomy comparator from
docs/playground/picker.js and remove the redundant sort in buildModelTree, since
its entries come from an already-sorted select. In docs/playground/app.js,
import and reuse compareByTaxonomy when constructing ordered instead of
maintaining an inline comparator.
In `@scripts/gen_model_docs.py`:
- Around line 430-433: Update the render_gallery docstring and adjacent taxonomy
comment to describe the current two-level family/subgroup grouping and
corresponding heading-level contract, replacing the stale “grouped by category”
terminology. Remove the historical “renders exactly as before” comparison while
retaining the explanation that registry ordering and consecutive grouping
rebuild the taxonomy.
🪄 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: e24ea139-dd5c-4937-949b-8d09d549bde2
⛔ Files ignored due to path filters (17)
docs/playground/data/b1_afi.nii.gzis excluded by!**/*.gzdocs/playground/data/b1_dam.nii.gzis excluded by!**/*.gzdocs/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 (75)
ci/integration_osf.shcrates/qmrust-cli/src/bidsify.rscrates/qmrust-cli/src/catalog.rscrates/qmrust-cli/src/commands.rscrates/qmrust-cli/src/io/nifti.rscrates/qmrust-cli/src/main.rscrates/qmrust-core/src/models/b1_afi/config.rscrates/qmrust-core/src/models/b1_afi/fit.rscrates/qmrust-core/src/models/b1_afi/mod.rscrates/qmrust-core/src/models/b1_afi/model.rscrates/qmrust-core/src/models/b1_dam/config.rscrates/qmrust-core/src/models/b1_dam/fit.rscrates/qmrust-core/src/models/b1_dam/mod.rscrates/qmrust-core/src/models/b1_dam/model.rscrates/qmrust-core/src/models/mod.rscrates/qmrust-core/src/registry.rscrates/qmrust-wasm/src/bids.rscrates/rust-bids/src/default_grouping.yamlcrates/rust-bids/src/inputs.rscrates/rust-bids/src/lib.rscrates/rust-bids/src/vocab.rsdocs/figures/b1_afi/inputs.webpdocs/figures/b1_afi/outputs.webpdocs/figures/b1_dam/curve.webpdocs/figures/b1_dam/inputs.webpdocs/figures/b1_dam/outputs.webpdocs/figures/inversion_recovery/inputs.webpdocs/figures/mono_t2/inputs.webpdocs/figures/mt_ratio/inputs.webpdocs/figures/mt_sat/inputs.webpdocs/figures/vfa_t1/inputs.webpdocs/index.mddocs/models/field-mapping/b1_afi.mddocs/models/field-mapping/b1_dam.mddocs/models/index.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/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/sources.jsondocs/playground/data/vfa_t1.jsondocs/playground/index.htmldocs/playground/picker.jsdocs/playground/recipe.jsdocs/playground/segment.jsdocs/playground/state.jsdocs/playground/vendor/icons.jsdocs/playground/volume.jsdocs/references.bibrecipes/bids/b1_afi_config.yamlrecipes/bids/b1_dam_config.yamlrecipes/bids/mt_sat_b1corr_config.yamlrecipes/bids/mt_sat_config.yamlrecipes/non-bids/b1_afi_config.yamlrecipes/non-bids/b1_dam_config.yamlscripts/docsfig/dataset.pyscripts/gen_model_docs.pyscripts/make_bids_examples.shscripts/make_docs_figures.pyscripts/tests/model_tree.test.mjsscripts/tests/test_dataset.pyscripts/tests/test_gen_model_docs.pyscripts/tests/volume_geometry.test.mjs
Comment on lines
+246
to
+264
| /// The datatype directory a BIDS suffix belongs in. | ||
| /// | ||
| /// BIDS files a suffix under one datatype wherever it appears, so this is a | ||
| /// property of the suffix alone — the same answer for a raw acquisition, a | ||
| /// preprocessed input, and a derivative output. Transmit- and receive-field | ||
| /// suffixes (`TB1*`, `RB1*`) and B0 field maps are `fmap`; everything else a | ||
| /// qMRI model reads or writes (weighted series, relaxation and MT maps, brain | ||
| /// masks) is `anat`. | ||
| pub fn datatype_for_suffix(suffix: &str) -> &'static str { | ||
| if suffix.starts_with("TB1") | ||
| || suffix.starts_with("RB1") | ||
| || suffix.ends_with("B0map") | ||
| || suffix == "fieldmap" | ||
| { | ||
| "fmap" | ||
| } else { | ||
| "anat" | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files 'crates/rust-bids/src/vocab.rs' 'crates/qmrust-cli/src/bidsify.rs' 'crates/qmrust-cli/src/commands.rs'
printf '%s\n' '--- vocab.rs structure ---'
ast-grep outline crates/rust-bids/src/vocab.rs
printf '%s\n' '--- classifier and tests ---'
rg -n -C 12 'datatype_for_suffix|field_map_suffixes_are_filed_under_fmap_everything_else_under_anat' crates/rust-bids/src/vocab.rs
printf '%s\n' '--- all classifier call sites ---'
rg -n -C 5 'datatype_for_suffix' crates
printf '%s\n' '--- suffix definitions and related BIDS routing ---'
rg -n -C 4 '"(epi|phasediff|phase1|phase2|magnitude|magnitude1|magnitude2|fieldmap|B0map)"|suffix|datatype' crates/rust-bids crates/qmrust-cliRepository: qMRLab/qmrust
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- vocabulary constants and implementation context ---'
sed -n '1,180p' crates/rust-bids/src/vocab.rs
sed -n '230,320p' crates/rust-bids/src/vocab.rs
printf '%s\n' '--- model-declared suffixes ---'
rg -n 'bids_suffix|suffix:|bids_outputs|bids:|phasediff|phase1|phase2|magnitude1|magnitude2|(^|[^A-Za-z])epi([^A-Za-z]|$)' crates/qmrust-core crates/qmrust-cli crates/rust-bids -g '*.rs' | head -300
printf '%s\n' '--- focused writer tests and output-path documentation ---'
sed -n '500,575p' crates/qmrust-cli/src/commands.rs
sed -n '940,1030p' crates/qmrust-cli/src/bidsify.rs
printf '%s\n' '--- deterministic classifier probe from the source predicate ---'
python3 - <<'PY'
suffixes = [
"TB1DAM", "TB1map", "TB1EPI", "RB1COR", "B0map", "fieldmap",
"epi", "phasediff", "phase1", "phase2", "magnitude", "magnitude1",
"magnitude2", "IRT1", "MESE", "MTR", "MTS", "VFA", "QMTSPGR",
"T1map", "MTsat",
]
def datatype_for_suffix(suffix):
return (
"fmap"
if suffix.startswith("TB1")
or suffix.startswith("RB1")
or suffix.endswith("B0map")
or suffix == "fieldmap"
else "anat"
)
for suffix in suffixes:
print(f"{suffix}: {datatype_for_suffix(suffix)}")
PYRepository: qMRLab/qmrust
Length of output: 35151
🌐 Web query:
BIDS specification entity table fieldmap suffixes epi phasediff phase1 phase2 magnitude magnitude1 magnitude2 datatype fmap
💡 Result:
In the Brain Imaging Data Structure (BIDS) specification, the fmap datatype is used for storing fieldmap-related data [1][2]. Within this directory, various suffixes are defined to describe the specific type of imaging data acquired for field correction or mapping [3]. The following suffixes are commonly associated with the fmap datatype [3][4][5]: epi: Used for Echo Planar Imaging data, often involving multiple spin-echo EPI scans with different phase-encoding directions to estimate field inhomogeneities [3]. phasediff: Represents a phase-difference map, commonly generated by subtracting a phase1 map from a phase2 map [3]. phase1: A phase map associated with the first echo in a GRE or similar sequence [3]. phase2: A phase map associated with the second echo in a GRE or similar sequence [3]. magnitude: A magnitude map used for anatomical reference in gradient-recalled echo (GRE) field-mapping schemes [3]. magnitude1: A magnitude map associated with the first echo in a sequence [3]. magnitude2: A magnitude map associated with the second echo in a sequence [3]. fieldmap: Used for files that directly provide maps of the B0 field inhomogeneity, such as those from spiral-echo imaging (SEI) sequences [3]. These suffixes categorize the raw data files (.nii or.nii.gz) and their corresponding sidecar JSON files stored within the fmap/ directory of a BIDS dataset [1][2]. Note that the phase suffix is deprecated and should instead be represented using the part entity (e.g., part-phase) in conjunction with other identifiers [3][6].
Citations:
- 1: https://bids-website.readthedocs.io/en/latest/getting_started/folders_and_files/files.html
- 2: https://bids.neuroimaging.io/getting_started/folders_and_files/files.html
- 3: https://github.com/bids-standard/bids-specification/blob/master/src/schema/objects/suffixes.yaml
- 4: https://bids-specification.readthedocs.io/en/v1.10.0/appendices/entity-table.html
- 5: https://bids-standard.github.io/bids-specification-ignore/src/99-appendices/04-entity-table.html
- 6: https://github.com/bids-standard/bids-specification/blob/master/src/schema/objects/entities.yaml
Map standard field-map suffixes to fmap, or narrow the API contract.
datatype_for_suffix returns anat for canonical field-map suffixes such as epi, phasediff, phase1, phase2, magnitude, magnitude1, and magnitude2. Add these cases and regression tests. If the API is qMRI-only, state that contract clearly and prevent generic BIDS suffixes from reaching it.
🤖 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 246 - 264, Update
datatype_for_suffix to classify the canonical field-map suffixes epi, phasediff,
phase1, phase2, magnitude, magnitude1, and magnitude2 as fmap alongside the
existing field-map cases, and add regression tests covering them. If the
function is intentionally qMRI-only instead, clearly narrow its documented
contract and ensure generic BIDS suffixes cannot reach it.
Source: Coding guidelines
Two are defects with reproductions: A protocol value of NaN passed every guard, because `x <= 0.0` is false for NaN. It reached a sidecar as `null` and fitted an all-NaN map. The guards are now stated positively. A partially stated protocol panicked: `repetition_times: [0.02]` indexed `[1]` in `B1AfiFitter::tr1`. Both models now reject an array that is neither empty nor the pair the method is defined on. Both checks live in `validate_options`, not `validate_protocol`: they are properties of the values themselves, and `describe` (which `bidsify` uses) runs only the former, so `validate_protocol` never saw them. The rest: * The derivative example in the generated pages said `anat/` for every model. The datatype now comes from the catalog, per output, from the same rule the writers use, so a field map documents `fmap/`. * Diagnostic outputs were described as written alongside the quantitative maps. They are not written as BIDS derivatives at all. * A mask warning naming a suffix the dataset lacks says which suffix, and which collection it is about. * `datatype_for_suffix` documents its scope: the suffixes qmrust's models declare. The generic field-map suffixes get no branch because no registered model names one, and unreachable branches would be worse than the gap. * The picker's nested rows sit in `role="group"` containers. * One taxonomy comparator, shared by the tree and the select it mirrors. * Unit coverage for the per-volume ingestion path: stack order, dimension mismatch, header retention, and the volume-count contract. Not done: adding `epi`/`phasediff`/`magnitude*` to `datatype_for_suffix`. All three call sites are fed model-declared suffixes only, so those branches are unreachable today.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-cli/src/io/nifti.rs`:
- Around line 320-325: Update stacking_follows_the_order_the_paths_were_given
and the related header test to create distinguishable fixtures: write different
voxel values for each input and assign distinct spatial-header values. Add
assertions verifying stacked volumes retain input order and that the resulting
header matches the first file’s spatial-header values, rather than allowing
identical fixtures to mask reordering or incorrect header selection.
🪄 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: 2af2e88e-d056-48e9-8aad-f275d631df1d
⛔ Files ignored due to path filters (17)
docs/playground/data/b1_afi.nii.gzis excluded by!**/*.gzdocs/playground/data/b1_dam.nii.gzis excluded by!**/*.gzdocs/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 (16)
crates/qmrust-cli/src/bidsify.rscrates/qmrust-cli/src/catalog.rscrates/qmrust-cli/src/commands.rscrates/qmrust-cli/src/io/nifti.rscrates/qmrust-core/src/models/b1_afi/config.rscrates/qmrust-core/src/models/b1_dam/config.rscrates/rust-bids/src/inputs.rscrates/rust-bids/src/vocab.rsdocs/models/field-mapping/b1_afi.mddocs/models/field-mapping/b1_dam.mddocs/models/magnetization-transfer/qmt_spgr.mddocs/models/t1-relaxometry/inversion_recovery.mddocs/playground/app.jsdocs/playground/picker.jsscripts/gen_model_docs.pyscripts/tests/test_gen_model_docs.py
🚧 Files skipped from review as they are similar to previous changes (13)
- docs/models/t1-relaxometry/inversion_recovery.md
- docs/models/magnetization-transfer/qmt_spgr.md
- docs/models/field-mapping/b1_dam.md
- scripts/tests/test_gen_model_docs.py
- crates/rust-bids/src/vocab.rs
- crates/rust-bids/src/inputs.rs
- scripts/gen_model_docs.py
- crates/qmrust-core/src/models/b1_dam/config.rs
- crates/qmrust-cli/src/catalog.rs
- docs/playground/app.js
- docs/models/field-mapping/b1_afi.md
- crates/qmrust-cli/src/bidsify.rs
- crates/qmrust-cli/src/commands.rs
Comment on lines
+320
to
+325
| fn stacking_follows_the_order_the_paths_were_given() { | ||
| let dir = TempDir::new("stack-order"); | ||
| let a = write_nifti(&dir.0, "a.nii", &[2, 3, 1]); | ||
| let b = write_nifti(&dir.0, "b.nii", &[2, 3, 1]); | ||
| let (fwd, _) = stack_nii_volumes(&[a.clone(), b.clone()]).unwrap(); | ||
| let (rev, _) = stack_nii_volumes(&[b, a]).unwrap(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the test fixtures distinguish the input files.
write_nifti gives a and b identical voxel values and identical default headers. The order test passes if stack_nii_volumes reorders the paths. The header test passes if it returns the second header.
Give each file distinct voxel values. Give each file distinct spatial-header values. Then assert the expected volume values and the first file’s header values.
Also applies to: 350-355
🤖 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/nifti.rs` around lines 320 - 325, Update
stacking_follows_the_order_the_paths_were_given and the related header test to
create distinguishable fixtures: write different voxel values for each input and
assign distinct spatial-header values. Add assertions verifying stacked volumes
retain input order and that the resulting header matches the first file’s
spatial-header values, rather than allowing identical fixtures to mask
reordering or incorrect header selection.
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 two B1+ mapping models and the shell work they turned up, plus a
two-level method taxonomy that both the docs gallery and the playground picker
derive from.
The models
b1_dam(double angle,TB1DAM) andb1_afi(actual flip angle,TB1AFI),both closed form. Each is validated voxelwise against qMRLab's own
FitResultson its OSF example data:
b1_damb1_afiReproducing MATLAB's arithmetic literally is what makes those last two columns
match.
b1_afipins an unphysical signal ratio to zero withcos_arg*(r<=1) + 1*(r>1), whose IEEE behaviour is load-bearing:Inf*0andNaN*0are both NaN, so a non-finite ratio stays NaN rather than being pinned.An
if/elseclamp would have looked equivalent and silently moved 103484voxels.
BIDS entities come from the spec's schema rather than convention:
TB1DAMhasflip: required,TB1AFIis anRFFieldMapssuffix indexed byacq-tr1/acq-tr2.Shell changes the ports required
None of these are per-model branches:
--nii-datais repeatable, so aSeriesmodel whose dataset ships one 3Dfile per volume reads directly.
read_named_nii_volumesnow delegates to ashared
stack_nii_volumes.datatype_for_suffixis the singlefmap/anatrule, replacing a helperthat knew only
*B1map/*B0map, and it drives all three writers. A modelwriting
TB1mapintoanat/is invisible to the B1 resolver that readsfmap/.qformnorsformdefines no geometry, soit is replaced rather than propagated. qMRLab's
b1_damexample declares a2 micrometre slice, which renders as an empty sliver. Voxel data is untouched.
mask:block now warns. It resolved silently before, and therecipe is echoed verbatim into the provenance
Parameters, so an unmaskedfit's sidecar read exactly like a masked one. A blank entity is now
unconstrained rather than a match on the empty string, which lets recipes for
datasets with no mask leave
descempty instead of naming one that is notthere.
Taxonomy and playground
Categorygainsfamily,subgroup,orderandicon. Both the gallery andthe picker sort by
orderand group consecutive runs, so the tree is statedonce. The playground picker is a collapsible two-level tree; the
<select>remains the value and the tree mirrors it, so a dropped dataset selecting a
model still works and the control degrades to a native select.
Also:
growMaskfor shrinking/growing a computed mask (skipping axes of extent1, since four of five example datasets are one slice thick and counting the
absent z would erase the mask on the first shrink), and clearer copy on the
segment method and protocol slider.
Notes for review
models/relaxometry/{inversion_recovery,mono_t2,vfa_t1}.mdare now under
t1-relaxometry/andt2-relaxometry/. The MT pages do notmove: both MT categories deliberately share one directory, with a test
asserting a shared slug can never span two families.
sources.jsonpoints at Zenodo record21753826, already uploaded. Alleight archives resolve 200 with
Access-Control-Allow-Origin: *.b1_afihas no forward curve in the playground, because thecurve needs every
param_nameand its T1 is never fitted.mt_satis skippedfor the same structural reason.
b0_demwas assessed and deliberately not ported: it needs a whole-volume fitpath (
FitStrategy::MatrixWiseis a seam only) and a spatial phase unwrapper,so it is an architecture task rather than a model port.
Verification
424 Rust tests, 124 node, 26 python,
cargo fmt --check,clippy -D warningsclean, both wasm purity builds, source-hygiene, theme-contrast and
gen_model_docs --check. Every one of the seven commits builds and testsstandalone.
https://claude.ai/code/session_01JDh9nrWyw1ju5TZ8rLp4dj
Summary by CodeRabbit