Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 80 additions & 2 deletions .claude/skills/porting-qmrlab-models/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ doubt, ask — that is cheaper at every phase than discovering it at validation.
(or read it from a configured location) and confirm the target model name. It reads
`.m` files directly with Read/Grep. Record how this model fetches its example data
(an `onlineData` URL / demo `*_batch.m` / `qMRgenBatch` download) — the port depends
on it for the bidsify and validation gates.
on it for the bidsify, validation, and app-integration gates.

**Phase 1 — Read the class.** Locate `equations`/`fit`/`Prot`/`xnames`/`st,lb,ub,fx`
and options in the `.m` files. Produce a written statement of the signal equation,
Expand All @@ -66,6 +66,36 @@ with their units.
`TODO(port)` markers in `config.rs` (config fields), `fit.rs` (signal equation +
fitter), `model.rs` (protocol mapping / `bids()`), and the `default_grouping.yaml`
grouping block. Write the forward→fit round-trip test.

For a `Series` model, **the per-volume protocol params *are* the volume
identity** (`engine::build_volume_ids` builds them from `proto.volumes`), and
`forward` must tag its samples with exactly the same keys. So a quantity that is
constant across the series — a single TR, a fixed TE — belongs in
`Scope::Global`, not `Scope::PerVolume`: as a per-volume param it silently joins
the identity on one side only, and no predicted sample matches its volume. The
fit keeps working, because it queries one key by name; the only symptom is a
missing forward curve in the app. Assert the two agree, don't eyeball it:
```rust
// Compared by volume index: any drift between the protocol-derived identity
// and the sample `forward` emits for that volume fails here, immediately.
let ids = crate::engine::build_volume_ids(m.measurement(), &proto, m.n_volumes()).unwrap();
let sig = m.forward(&params, &Aux::new());
let samples = sig.series();
assert_eq!(samples.len(), ids.len());
for (id, sample) in ids.iter().zip(samples) {
let crate::core::model::VolumeId::Params(row) = id else {
panic!("a Series model must yield param-row identities")
};
assert_eq!(
*row, sample.params,
"volume identity {row:?} has no matching forward sample {:?}",
sample.params
);
}
```
Comment thread
coderabbitai[bot] marked this conversation as resolved.
`models/vfa_t1/model.rs` carries this as
`forward_samples_carry_the_volume_identities_the_bids_path_builds`; copy it and
swap the model.
→ *Gate: confirm the translation; round-trip test passes.*

**Phase 3 — Wire.** Confirm the registry line and grouping block the scaffold added;
Expand Down Expand Up @@ -94,6 +124,49 @@ per-volume axis that `Protocol` already records from the sidecars.
`FitResults` for the same dataset.
→ *Gate: human reviews the delta and signs off.*

**Phase 6 — Ship it in the app.** A model nobody can reach in the playground is
half-ported. The playground (`docs/playground/`) and `qmrust-wasm` are entirely
data-driven — no per-model JS, HTML, CSS, or wasm binding exists, and none should
be added. What the app needs is the model's *example dataset and payload*, both
generated:

1. Add the model's block to `scripts/make_bids_examples.sh` — an OSF `fetch`
line, and a `ds-<lowercased suffix>` section that bidsifies, fits, and
`assert_maps`-checks its outputs. Roots are named from the registry suffix, so
there is no model→directory table to edit. Run it with `--zip`.
2. Regenerate the catalog and the playground payload:
```bash
./target/release/qmrust catalog > catalog.json
python3 scripts/make_docs_figures.py --bids-dir <dataset parent> \
--catalog catalog.json --bundle-slice
```
This writes `docs/figures/<model>/*.webp`, `docs/playground/data/<model>.json`
(+ its `.nii.gz` slices) and re-derives `docs/playground/data/index.json`,
which is what the model picker reads. A model missing from `index.json` means
its dataset root wasn't found — fix step 1, don't hand-edit the manifest.
3. Check the generated payload's `files.aux` lists every optional input the model
declares, and that its `probes` agree with the CLI's own maps. An aux map that
silently fails to resolve makes the app fit uncorrected while the CLI corrects
— the maps then differ with nothing to point at.
4. **Rebuild the playground wasm before serving locally.** No wasm *source* edit
is needed, but `docs/playground/pkg/` is a gitignored build artifact: a stale
one carries the old registry and the app fails with `Unknown model: '<name>'`
while still rendering the pre-baked slice, which reads like a data problem and
is not. CI rebuilds it on deploy, so this bites local testing only.
```bash
cd crates/qmrust-wasm && wasm-pack build --target web --out-dir ../../docs/playground/pkg
```
Then hard-reload — browsers cache the `.wasm`.
5. Upload `ds-<suffix>.zip` to the Zenodo deposition behind
`docs/playground/data/sources.json` (the fetch path resolves
`${base}/${archive}${suffix}`). Adding a file mints a **new version id**, so
afterwards update `record`/`doi`/`base` to that id — the concept id 404s on
the `/api/` files route. Then confirm every model's archive still resolves
200 with `Access-Control-Allow-Origin: *`, not just the new one. **This
publishes data externally — confirm with the user before uploading.**
→ *Gate: the model appears in `docs/playground/data/index.json`, its payload has
the expected aux + probes, and the user has confirmed the archive upload.*

## Definition of done

Always required:
Expand All @@ -113,7 +186,12 @@ qMRLab model defines how to fetch it):
suffix, per-volume entities, and sidecars match `bids()`/`bids_volume()`, voxel data
byte-identical to source;
- the model fits that bidsified data via `qmrust fit --bids-dir` with the BIDS recipe,
and its output provenance does not duplicate the acquisition axis into `Parameters`.
and its output provenance does not duplicate the acquisition axis into `Parameters`;
- the model is reachable in the playground: a `scripts/make_bids_examples.sh` block
builds its `ds-<suffix>` root, and a regenerated
`docs/playground/data/{index.json,<model>.json}` (+ slices) carries its aux maps and
CLI-agreeing probes. No per-model JS/HTML/CSS/wasm edit is ever part of this — if one
seems necessary, the app has grown a special case that belongs in the data instead.

Required when a qMRLab reference result exists (e.g. `FitResults` on OSF):

Expand Down
41 changes: 40 additions & 1 deletion ci/integration_osf.sh
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,13 @@ curl -L --fail -o "$DATA/qmt.zip" "https://osf.io/pzqyn/download?version=2"
curl -L --fail -o "$DATA/mono_t2.zip" "https://osf.io/kujp3/download?version=3"
curl -L --fail -o "$DATA/mtr.zip" "https://osf.io/erm2s/download?version=2"
curl -L --fail -o "$DATA/mtsat.zip" "https://osf.io/c5wdb/download?version=4"
curl -L --fail -o "$DATA/vfa_t1.zip" "https://osf.io/7wcvh/download?version=3"
unzip -o -q "$DATA/ir.zip" -d "$DATA/ir"
unzip -o -q "$DATA/qmt.zip" -d "$DATA/qmt"
unzip -o -q "$DATA/mono_t2.zip" -d "$DATA/mono_t2"
unzip -o -q "$DATA/mtr.zip" -d "$DATA/mtr"
unzip -o -q "$DATA/mtsat.zip" -d "$DATA/mtsat"
unzip -o -q "$DATA/vfa_t1.zip" -d "$DATA/vfa_t1"

# Locate the datasets by their key files (robust to archive folder layout).
IR_MAT="$(find "$DATA/ir" -name 'IRData.mat' | head -1)"
Expand Down Expand Up @@ -58,6 +60,19 @@ MTSAT_REF_MTR="$(find "$DATA/mtsat" -path '*FitResults*' -name 'MTR.nii.gz' | he
[ -n "$MTSAT_REF_SAT" ] || { echo "FitResults/MTSAT.nii.gz not found in MTsat archive"; exit 1; }
MTSAT_DIR="$(dirname "$MTSAT_MTW")"

# vfa_t1 ships as a 4D NIfTI (VFAData.nii.gz, flip angles in the 4th axis) plus
# a transmit field map and a mask, with a qMRLab reference (FitResults/).
VFA_DATA="$(find "$DATA/vfa_t1" -name 'VFAData.nii.gz' | head -1)"
VFA_B1="$(find "$DATA/vfa_t1" -name 'B1map.nii.gz' | head -1)"
VFA_MASK="$(find "$DATA/vfa_t1" -name 'Mask.nii.gz' | head -1)"
VFA_REF_T1="$(find "$DATA/vfa_t1" -path '*FitResults*' -name 'T1.nii.gz' | head -1)"
VFA_REF_M0="$(find "$DATA/vfa_t1" -path '*FitResults*' -name 'M0.nii.gz' | head -1)"
[ -n "$VFA_DATA" ] || { echo "VFAData.nii.gz not found in VFA archive"; exit 1; }
[ -n "$VFA_B1" ] || { echo "B1map.nii.gz not found in VFA archive"; exit 1; }
[ -n "$VFA_MASK" ] || { echo "Mask.nii.gz not found in VFA archive"; exit 1; }
[ -n "$VFA_REF_T1" ] || { echo "FitResults/T1.nii.gz not found in VFA archive"; exit 1; }
[ -n "$VFA_REF_M0" ] || { echo "FitResults/M0.nii.gz not found in VFA archive"; exit 1; }

echo "Running IR fit..."
"$BIN" fit --mat-data "$IR_MAT" --mask "$IR_MASK" \
--config recipes/non-bids/irt1_config.yaml --output-dir "$DATA/out_ir"
Expand Down Expand Up @@ -99,12 +114,25 @@ echo "Running mt_sat bidsify + BIDS-path fit..."
"$BIN" fit --bids-dir "$DATA/mtsat_bids" \
--config recipes/bids/mt_sat_config.yaml --output-dir "$DATA/out_mtsat"

echo "Running vfa_t1 bidsify + BIDS-path fit..."
# bidsify reads the 4D series from --nii-data and the transmit map via --aux;
# the non-BIDS recipe supplies the flip angles and TR, written into the VFA
# sidecars. The BIDS-path fit folds those sidecars back in and resolves the
# TB1map derivative as the model's optional B1map input.
"$BIN" bidsify --model vfa_t1 --nii-data "$VFA_DATA" --nii-mask "$VFA_MASK" \
--aux "B1map=$VFA_B1" \
--config recipes/non-bids/vfa_t1_config.yaml --subject 01 --out "$DATA/vfa_t1_bids"
"$BIN" fit --bids-dir "$DATA/vfa_t1_bids" \
--config recipes/bids/vfa_t1_config.yaml --output-dir "$DATA/out_vfa_t1"

echo "Asserting outputs..."
for f in "$DATA/out_ir/T1.nii.gz" "$DATA/out_ramani/F.nii.gz" "$DATA/out_srp/F.nii.gz" \
"$DATA/out_mono_t2/qmrust/sub-01/anat/sub-01_T2map.nii.gz" \
"$DATA/out_mtr/qmrust/sub-01/anat/sub-01_MTRmap.nii.gz" \
"$DATA/out_mtsat/qmrust/sub-01/anat/sub-01_MTsat.nii.gz" \
"$DATA/out_mtsat/qmrust/sub-01/anat/sub-01_T1map.nii.gz"; do
"$DATA/out_mtsat/qmrust/sub-01/anat/sub-01_T1map.nii.gz" \
"$DATA/out_vfa_t1/qmrust/sub-01/anat/sub-01_T1map.nii.gz" \
"$DATA/out_vfa_t1/qmrust/sub-01/anat/sub-01_M0map.nii.gz"; do
test -s "$f" || { echo "MISSING or empty: $f"; exit 1; }
done

Expand Down Expand Up @@ -141,4 +169,15 @@ 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

# vfa_t1 is a closed-form linearized least-squares solve with the same B1 map
# qMRLab used, and both report T1 in seconds, so every masked voxel agrees to
# the float32 precision the maps are stored in.
echo "Comparing vfa_t1 maps to qMRLab FitResults..."
python3 ci/compare_maps.py \
"$DATA/out_vfa_t1/qmrust/sub-01/anat/sub-01_T1map.nii.gz" "$VFA_REF_T1" \
--rel-tol 0.001 --min-frac 0.999 --min-corr 0.999 --label vfa_t1-T1
python3 ci/compare_maps.py \
"$DATA/out_vfa_t1/qmrust/sub-01/anat/sub-01_M0map.nii.gz" "$VFA_REF_M0" \
--rel-tol 0.001 --min-frac 0.999 --min-corr 0.999 --label vfa_t1-M0

echo "OSF integration OK"
60 changes: 51 additions & 9 deletions crates/qmrust-cli/src/bidsify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ fn measurement_in_dir(dir: &Path, model: &dyn Model) -> Result<PathBuf> {

/// Write one dataset as a BIDS tree rooted at `out`, driven by
/// `model`'s declared BIDS identity: `dataset_description.json`,
/// `participants.tsv`, a `.bidsignore` entry if `model`'s suffix is
/// `participants.tsv`, a `.bidsignore` entry iff `model`'s suffix is
/// non-canonical, one `sub-<subject>/anat/sub-<subject>[_<entity>-<val>...]_<suffix>.nii.gz`
/// (+ JSON sidecar, from `model.bids_volume(i)`) per volume in `data`'s 4th
/// axis, a brain-mask derivative if `mask` is given, and each `aux` map
Expand Down Expand Up @@ -312,7 +312,7 @@ pub fn write_bids_tree(
std::fs::create_dir_all(out)?;
write_dataset_description(out)?;
write_participants_row(out, subject)?;
write_bidsignore_if_custom(out, spec.suffix)?;
sync_bidsignore(out, spec.suffix)?;

let anat_dir = out.join(format!("sub-{subject}")).join("anat");
std::fs::create_dir_all(&anat_dir)?;
Expand Down Expand Up @@ -492,17 +492,35 @@ fn write_derivative_dataset_description(pipeline: &Path, name: &str) -> Result<(
Ok(())
}

/// Ensure the dataset root's `.bidsignore` contains a `*<suffix>*` line when
/// `suffix` is not a canonical BIDS suffix (a registered model's own
/// suffix is "custom" by construction — see `rust_bids::Vocabulary`).
/// Canonical suffixes need no entry. Creates the file if missing; never
/// Reconcile the dataset root's `.bidsignore` with `suffix`'s status: a
/// non-canonical suffix (like `QMTSPGR`) gets a `*<suffix>*` line so generic
/// BIDS validators don't choke on it, and a canonical one (like `VFA`) must
/// have no such line — an entry there would hide the acquisitions from any
/// validator that honours it. Both directions are enforced, so appending to a
/// root written before a suffix's status was settled corrects it rather than
/// leaving the stale line in place. Creates the file if missing; never
/// duplicates the line.
fn write_bidsignore_if_custom(out: &Path, suffix: &str) -> Result<()> {
fn sync_bidsignore(out: &Path, suffix: &str) -> Result<()> {
let path = out.join(".bidsignore");
let line = format!("*{suffix}*");

if !rust_bids::Vocabulary::bids().is_custom_suffix(suffix) {
if !path.exists() {
return Ok(());
}
let contents = std::fs::read_to_string(&path)?;
let kept: Vec<&str> = contents.lines().filter(|l| l.trim() != line).collect();
if kept.len() == contents.lines().count() {
return Ok(());
}
if kept.iter().all(|l| l.trim().is_empty()) {
std::fs::remove_file(&path)?;
} else {
std::fs::write(&path, format!("{}\n", kept.join("\n")))?;
}
return Ok(());
}
let path = out.join(".bidsignore");
let line = format!("*{suffix}*");

if !path.exists() {
std::fs::write(&path, format!("{line}\n"))?;
return Ok(());
Expand Down Expand Up @@ -981,4 +999,28 @@ mod tests {

let _ = std::fs::remove_dir_all(&dir);
}

/// A canonical BIDS suffix must never be `.bidsignore`d — an entry there
/// hides the acquisitions from any validator that honours it. Writing into
/// a root that already carries a stale line must clear it, while leaving
/// unrelated entries alone.
#[test]
fn bidsify_clears_a_stale_bidsignore_line_for_a_canonical_suffix() {
let dir = tmp_dir("vfa-bidsignore-canonical");
std::fs::create_dir_all(&dir).unwrap();
std::fs::write(dir.join(".bidsignore"), "*VFA*\n*SOMETHINGELSE*\n").unwrap();

let v: serde_yaml::Value =
serde_yaml::from_str("model: vfa_t1\nflip_angles: [3, 20]\nrepetition_time: 0.015\n")
.unwrap();
let model = qmrust_core::models::vfa_t1::describe(&v).unwrap();
let data = Array4::from_shape_fn((1, 1, 1, 2), |_| 1.0);
write_bids_tree(model.as_ref(), &data, None, &[], "01", &dir, None).unwrap();

let bidsignore = std::fs::read_to_string(dir.join(".bidsignore")).unwrap();
assert!(!bidsignore.contains("*VFA*"), "{bidsignore}");
assert!(bidsignore.contains("*SOMETHINGELSE*"), "{bidsignore}");

let _ = std::fs::remove_dir_all(&dir);
}
}
1 change: 1 addition & 0 deletions crates/qmrust-core/src/models/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@ pub mod mono_t2;
pub mod mt_ratio;
pub mod mt_sat;
pub mod qmt_spgr;
pub mod vfa_t1;
Loading
Loading