Land the stranded refactor, write down the rules, and fix what they exposed - #30
Merged
Conversation
…er model Six properties were copy-pasted across the model test modules, 31 copies in total, each asserting a rule that holds for every model. They were also strictly weaker than a loop: a model added without remembering to paste them was simply uncovered. They now live in tests/properties.rs, which already builds every registered model from its shipped recipe and exists for exactly this. Where a rule does not apply to all models the guard comes from what the model declares, never from its name: the identity properties run for `Series` measurements, and the acquisition property for models with a non-empty protocol_schema, so `mt_ratio` and the `Named` models exclude themselves. Two of the rules got stronger in the move. Describing "without an acquisition" is now done from each model's own BIDS recipe, which is what the --bids-dir path actually hands it, rather than from a bare `model:` line that only three models could parse. Order-invariance compares NaN-aware, so it covers models whose parameters put a forward at the edge of its domain. Verified by breaking identity-matching in one model and watching the shared property fail, then restoring it. One exception is named rather than papered over: qmt_spgr builds with no acquisition at all, because its config carries a default saturation grid. A BIDS fit whose sidecars went missing would silently fit that grid. Excluding it silently is how the gap stopped being visible. Also drops two tests of mine that only restated a literal from the impl a few lines above.
Five models are a Series indexed by one per-volume protocol key: inversion time, echo time, flip angle, excitation repetition time. Each reimplemented the same four things, differing only in the key's name — the identity rows its measurement declares, the tagged samples its forward emits, the signal its fit assembles back out, and the axis its ingest_protocol reads. `SeriesAxis` states the key once and provides all four. The third is the one that mattered: an assembly reading by position rather than by identity pairs every value with the wrong protocol row and produces a plausible wrong map rather than an error, and it was hand-written five times with five hand-written panic messages. qMT-SPGR keeps its own rows; it has two axes, so this does not apply, and the Named models have no axis at all. The four registry entry points were the same one-line delegation in every model, thirty-two functions carrying the same four doc comments and nothing model-specific. They are now `model_entry_points!(Config)`. The registry still stores plain fn pointers, so the shape it depends on is unchanged. Net across the models: 435 lines deleted for 186 added, against 141 added to the core. Behaviour is unchanged and checked as such rather than assumed: every example dataset was fitted before and after, and all 17 output maps across all 8 models hash byte-identically.
Collapse the repeated single-axis series into one abstraction
📝 WalkthroughWalkthroughThe PR adds shared model registry and single-axis series abstractions. Five models adopt ChangesModel abstractions and migrations
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/qmrust-core/src/core/model.rs (1)
170-204: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueQualify the
serde_yamlpath in the generated signatures.The generated bodies use absolute paths (
$crate::core::model::...,::anyhow::Result), but the parameter type is the unqualifiedserde_yaml::Value.#[macro_export]publishes this macro, so an invocation resolvesserde_yamlin the caller's module scope. Every current caller is insideqmrust-coreand resolves it, so behavior does not change today. Use::serde_yaml::Valueto make the macro self-contained and consistent with the other paths.♻️ Proposed refactor
pub fn describe( - v: &serde_yaml::Value, + v: &::serde_yaml::Value, ) -> ::anyhow::Result<Box<dyn $crate::core::model::Model>> { @@ pub fn build( - v: &serde_yaml::Value, + v: &::serde_yaml::Value, proto: &$crate::core::model::Protocol, ) -> ::anyhow::Result<Box<dyn $crate::core::model::Model>> { @@ - pub fn dump(v: &serde_yaml::Value) -> ::anyhow::Result<String> { + pub fn dump(v: &::serde_yaml::Value) -> ::anyhow::Result<String> { @@ pub fn effective( - v: &serde_yaml::Value, + v: &::serde_yaml::Value, proto: &$crate::core::model::Protocol, ) -> ::anyhow::Result<$crate::core::model::EffectiveConfig> {Note:
Boxis also unqualified but resolves through the prelude in any edition, so it is safe.🤖 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/core/model.rs` around lines 170 - 204, Update the generated function signatures in the model_entry_points macro to use the absolute ::serde_yaml::Value path for every YAML value parameter. Keep the existing $crate, ::anyhow, and Box references and macro behavior unchanged.
🤖 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/core/model.rs`:
- Around line 163-169: Update the doc block around the registry entry-point
macro in crates/qmrust-core/src/core/model.rs:163-169 to remove refactor history
and repetition counts, retaining only that the registry requires module-local
free functions and the macro generates them for one config type. Update the doc
block around assemble in crates/qmrust-core/src/core/model.rs:541-545 to remove
the retyping-history sentence while preserving the invariant that assembly
matches by identity value, never array position, because positional mapping can
silently produce an incorrect result.
---
Nitpick comments:
In `@crates/qmrust-core/src/core/model.rs`:
- Around line 170-204: Update the generated function signatures in the
model_entry_points macro to use the absolute ::serde_yaml::Value path for every
YAML value parameter. Keep the existing $crate, ::anyhow, and Box references and
macro behavior unchanged.
🪄 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: 5e0be9c8-e303-414a-86eb-49510be89e59
📒 Files selected for processing (10)
crates/qmrust-core/src/core/model.rscrates/qmrust-core/src/models/b1_afi/model.rscrates/qmrust-core/src/models/b1_dam/model.rscrates/qmrust-core/src/models/inversion_recovery/model.rscrates/qmrust-core/src/models/mono_t2/model.rscrates/qmrust-core/src/models/mt_ratio/model.rscrates/qmrust-core/src/models/mt_sat/model.rscrates/qmrust-core/src/models/qmt_spgr/adapter.rscrates/qmrust-core/src/models/vfa_t1/model.rscrates/qmrust-core/tests/properties.rs
Comment on lines
+163
to
+169
| /// A model module's four registry entry points, for the config type it owns. | ||
| /// | ||
| /// The registry stores these as plain function pointers, so each model needs | ||
| /// them as free functions in its own module; each is the same one-line | ||
| /// delegation to the shared pipeline, differing only in the config type. Eight | ||
| /// models times four functions was the same body and doc comment written | ||
| /// thirty-two times, with nothing model-specific to get right or wrong. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Both new doc blocks in crates/qmrust-core/src/core/model.rs narrate the refactor instead of the contract. Each one explains how the code used to be written per model and how many times it was repeated. The coding guidelines allow only the current contract, invariants, assumptions, and non-obvious reasoning in Rust documentation comments.
crates/qmrust-core/src/core/model.rs#L163-L169: delete the sentence counting eight models times four functions. State only that the registry needs free functions per module and that the macro generates them for one config type.crates/qmrust-core/src/core/model.rs#L541-L545: delete the sentence about the same code being retyped per model. Keep the invariant thatassemblematches by identity value and never by array position, because a position-based assembly produces a plausible wrong map instead of an error.
As per coding guidelines: "Comments, Rust documentation comments, and documentation must describe the current contract, invariants, assumptions, safety requirements, domain knowledge, and non-obvious reasoning; they must not describe history, rejected alternatives, review context, or task references."
📍 Affects 1 file
crates/qmrust-core/src/core/model.rs#L163-L169(this comment)crates/qmrust-core/src/core/model.rs#L541-L545
🤖 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/core/model.rs` around lines 163 - 169, Update the doc
block around the registry entry-point macro in
crates/qmrust-core/src/core/model.rs:163-169 to remove refactor history and
repetition counts, retaining only that the registry requires module-local free
functions and the macro generates them for one config type. Update the doc block
around assemble in crates/qmrust-core/src/core/model.rs:541-545 to remove the
retyping-history sentence while preserving the invariant that assembly matches
by identity value, never array position, because positional mapping can silently
produce an incorrect result.
Source: Coding guidelines
…the agent docs CLAUDE.md gains the rule the recent refactors were applying without it being written down: every piece of knowledge has one authoritative home, and everything else derives from it. Stated with the counterweight, because the rule is dangerous on its own: duplication is cheaper than the wrong abstraction, so the Rule of Three is the heuristic, immediate for a constant or a spec fact and slower than three for logic whose domain is still uncertain. The test named is whether two sites are the same *knowledge*, not the same characters. Four repo facts are listed as worked examples of a single home: Category, the BIDS datatype rule, SeriesAxis, and the shared model contract. Also the corollary the properties refactor turned up: a guard that cannot apply to everything derives from what the subject declares, never from its name. The agent docs had drifted from the code they describe: * ADDING-A-MODEL still told a new model to hand-write four registry entry points, which is now `model_entry_points!`, and to write per-model tests for contracts that are now asserted once over the registry. Following it would have reintroduced exactly the duplication just removed. * It said nothing about `SeriesAxis`, which is the whole identity handling for a single-axis series, nor about `Category` being a two-level taxonomy, nor about the datatype being decided by the suffix rather than by the model. * Its `Model` trait listing was missing `fit_block` and `sim_required_aux`. * Three derivative paths in ARCHITECTURE and DATA-PIPELINE still hardcoded `anat/`, which stopped being true when the datatype became suffix-driven; a fitted TB1map goes to `fmap/`. Checked rather than asserted: every path the docs cite exists, the documented trait now matches the real one method for method, and no doc claim about a shared helper is contradicted by a model still hand-rolling it.
agahkarakuzu
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/agents/ADDING-A-MODEL.md`:
- Around line 184-195: Update the SeriesAxis guidance in the model-adding
checklist to state that axis values must be unique and that SeriesAxis::assemble
panics when a declared identity lacks a matching sample. Alternatively, link
directly to the existing SeriesAxis contract while preserving the current method
guidance.
- Around line 43-50: Update the documentation around the model trait methods,
specifically the default fit_block behavior, to state that it calls fit for each
corresponding Measurement and Aux pair and returns results in input order;
alternatively, link to the relevant explanation in ARCHITECTURE.md.
In `@docs/agents/ARCHITECTURE.md`:
- Around line 295-298: Document that run_fit_bids passes output_dir directly to
write_derivatives as deriv_root, defining deriv_root as the BIDS --output-dir.
Update docs/agents/ARCHITECTURE.md lines 276 and 295-298 and
docs/agents/DATA-PIPELINE.md lines 316-318 to state this relationship and use
deriv_root consistently in the affected paths.
🪄 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: 2ec93000-ec15-43ba-a036-a8d97c50981b
📒 Files selected for processing (4)
CLAUDE.mddocs/agents/ADDING-A-MODEL.mddocs/agents/ARCHITECTURE.mddocs/agents/DATA-PIPELINE.md
Comment on lines
+43
to
+50
| fn sim_required_aux(&self) -> Vec<&'static str> { vec![] } | ||
| fn measurement(&self) -> MeasurementKind; | ||
|
|
||
| fn strategy(&self) -> FitStrategy { FitStrategy::Voxelwise } | ||
|
|
||
| fn forward(&self, params: &[f64], aux: &Aux) -> Measurement; | ||
| fn fit(&self, m: &Measurement, aux: &Aux) -> Vec<f64>; | ||
| fn fit_block(&self, ms: &[Measurement], aux: &[Aux]) -> Vec<Vec<f64>> { .. } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 '\bfn fit_block\b' crates/qmrust-core/src/core/model.rsRepository: qMRLab/qmrust
Length of output: 1211
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- documentation ---'
cat -n docs/agents/ADDING-A-MODEL.md | sed -n '35,55p'
printf '%s\n' '--- architecture contract ---'
rg -n -C 10 'fit_block|per.voxel|per voxel|each.*Measurement|each.*Aux' ARCHITECTURE.md docs crates/qmrust-core/src/core/model.rs
printf '%s\n' '--- related model documentation ---'
rg -n -C 6 'fit_block' docs crates --glob '*.md' --glob '*.rs'Repository: qMRLab/qmrust
Length of output: 23928
Document the default fit_block behavior.
State that the default implementation calls fit for each corresponding (Measurement, Aux) pair and returns results in input order, or link to docs/agents/ARCHITECTURE.md.
🤖 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/agents/ADDING-A-MODEL.md` around lines 43 - 50, Update the documentation
around the model trait methods, specifically the default fit_block behavior, to
state that it calls fit for each corresponding Measurement and Aux pair and
returns results in input order; alternatively, link to the relevant explanation
in ARCHITECTURE.md.
Source: Coding guidelines
Comment on lines
+184
to
+195
| (the hooks above), and `crate::model_entry_points!(<Name>Config);`, which | ||
| expands to the four functions the registry stores as fn pointers | ||
| (`build`/`describe`/`dump`/`effective`). Each is a one-line delegation to | ||
| the shared pipeline, so there is nothing model-specific to write. | ||
| - If the measurement is a `Series` identified by **one** per-volume protocol | ||
| key (inversion time, echo time, flip angle, excitation TR), declare it as | ||
| `const AXIS: SeriesAxis = SeriesAxis::new("<Key>");` and use | ||
| `AXIS.rows`/`AXIS.samples`/`AXIS.assemble`/`AXIS.ingest`. That is the | ||
| model's whole identity handling: rows for `measurement()`, tagged samples | ||
| for `forward`, the signal `fit` assembles by identity, and the axis | ||
| `ingest_protocol` reads. A model with two axes (qMT-SPGR: angle *and* | ||
| offset) owns its own rows. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the SeriesAxis identity invariants.
The checklist names the helper methods but omits two required assumptions: axis values must be unique, and assemble panics when a declared identity has no matching sample. Add these invariants or link directly to the SeriesAxis 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 `@docs/agents/ADDING-A-MODEL.md` around lines 184 - 195, Update the SeriesAxis
guidance in the model-adding checklist to state that axis values must be unique
and that SeriesAxis::assemble panics when a declared identity lacks a matching
sample. Alternatively, link directly to the existing SeriesAxis contract while
preserving the current method guidance.
Source: Coding guidelines
Comment on lines
+295
to
+298
| [/<session>]/<datatype>/<subject>[_<session>]_<Suffix>.nii.gz`, per each model's declared | ||
| `bids_outputs()`, with `<datatype>` decided by the suffix itself | ||
| (`rust_bids::datatype_for_suffix`: `anat/` for a tissue parameter, `fmap/` for a field | ||
| map) rather than by the model — and `qmrust bidsify` is the reverse direction, turning a qMRLab `.mat` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'output_dir/qmrust|deriv_root/qmrust|write_derivatives|output-dir' \
docs/agents/ARCHITECTURE.md docs/agents/DATA-PIPELINE.md cratesRepository: qMRLab/qmrust
Length of output: 9625
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- CLI and BIDS-fit path definitions ---'
rg -n -C 5 'struct .*Args|output_dir|deriv_root|run_fit_bids|bids-dir|output-dir' crates/qmrust-cli/src/commands.rs crates/qmrust-cli/src -g '*.rs' | head -n 240
printf '%s\n' '--- Documentation context ---'
sed -n '260,305p' docs/agents/ARCHITECTURE.md
sed -n '300,340p' docs/agents/DATA-PIPELINE.md
printf '%s\n' '--- Deterministic terminology check ---'
python3 - <<'PY'
from pathlib import Path
paths = [
Path("docs/agents/ARCHITECTURE.md"),
Path("docs/agents/DATA-PIPELINE.md"),
]
terms = ["output_dir/qmrust", "deriv_root/qmrust"]
for path in paths:
text = path.read_text()
print(path)
for term in terms:
print(f" {term}: {text.count(term)}")
PYRepository: qMRLab/qmrust
Length of output: 24490
Define deriv_root as the BIDS --output-dir.
run_fit_bids passes output_dir directly to write_derivatives as deriv_root. State this relationship in docs/agents/DATA-PIPELINE.md and use one root name consistently in the affected paths.
📍 Affects 2 files
docs/agents/ARCHITECTURE.md#L295-L298(this comment)docs/agents/ARCHITECTURE.md#L276-L276docs/agents/DATA-PIPELINE.md#L316-L318
🤖 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/agents/ARCHITECTURE.md` around lines 295 - 298, Document that
run_fit_bids passes output_dir directly to write_derivatives as deriv_root,
defining deriv_root as the BIDS --output-dir. Update docs/agents/ARCHITECTURE.md
lines 276 and 295-298 and docs/agents/DATA-PIPELINE.md lines 316-318 to state
this relationship and use deriv_root consistently in the affected paths.
Source: Coding guidelines
added 2 commits
August 2, 2026 17:35
…-keyed branch Three violations of the rules just written into CLAUDE.md, found by holding the code against them. `BidsSpec.entities` was a second declaration of which entities index a model's suffix, and it was never read. The grouping grammar in default_grouping.yaml is what actually assembles a collection; `Model::bids()` had one production consumer, which uses `.suffix` alone. So every model stated the same fact twice and the copy in the model could have said `Echo` while the grammar said `flip` with nothing to notice. `EntityRole` is gone and `BidsSpec` is the suffix. `if cfg.model == "mt_sat"` gated the b1_correction inlining in both the BIDS and non-BIDS fit paths. The guard was also redundant: the function already returns early when the recipe has no `b1_correction` key. It now runs unconditionally under a name that says what it does, applied by what the recipe declares rather than by which model declared it. Verified that exactly one shipped recipe carries the key, so behaviour is unchanged. `bidsify --help` still told users the subcommand was for "inversion_recovery" or "qmt_spgr". It has been registry-driven for some time and works for all eight. Fits are untouched, and checked rather than assumed: all 17 output maps across the 8 example datasets hash byte-identically to the pre-change baseline, and the b1-corrected mt_sat path still resolves its fitvalues artifact and fits.
ci/integration_osf.sh and scripts/make_bids_examples.sh each stated, for every model, the same three things: which OSF archive and pinned version it comes from, where its files sit inside that archive, and the exact bidsify invocation. Six models were duplicated verbatim; the two calls differed only in output directory. That is not merely repetitive. Bump `?version=` in one script and not the other and both stay green, while CI validates against data the reader never sees. The bytes CI compares to qMRLab's FitResults and the bytes shipped to the playground have to be the same bytes, and nothing was holding them together. ci/datasets.sh now owns all three, and each caller supplies only its output root and what it does afterwards: CI compares to FitResults, the builder zips. The mts-b1 phantom keeps its inline bidsify. It is not an OSF archive, occurs once, and shares only a subcommand name with the others; folding it in would be an abstraction over one case. Verified by running both scripts. All nine dataset roots rebuild to the same 213 files with all 93 volumes byte-identical, and every qMRLab comparison still runs and passes, including the two .mat-vs-BIDS round-trip tests.
agahkarakuzu
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/qmrust-core/src/core/model.rs (1)
568-579: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject mismatched series lengths.
zipstops at the shorter iterator. A short signal silently removes an axis identity, and a long signal is silently discarded. This violates theMeasurement::Seriescontract of one sample per axis value. Validate both lengths before building the measurement.Proposed fix
pub fn samples(&self, values: &[f64], signal: impl IntoIterator<Item = f64>) -> Measurement { + let signal: Vec<f64> = signal.into_iter().collect(); + assert_eq!( + values.len(), + signal.len(), + "series axis values and signal must have equal lengths" + ); Measurement::Series( values .iter() .zip(signal)🤖 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/core/model.rs` around lines 568 - 579, Update Model::samples to validate that the signal iterator produces exactly one value per entry in values before constructing Measurement::Series. Reject both shorter and longer signal inputs instead of relying on zip truncation, while preserving the existing Sample mapping for matching lengths.
🤖 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/datasets.sh`:
- Around line 24-31: Update fetch() so downloading and extraction are atomic:
track the archive and destination created for $name, and on any curl or unzip
failure remove the partially extracted directory and archive before returning
failure. Keep the existing cache-skip behavior only when $DATA/$name already
represents a successful extraction, allowing later fetch_all retries to
redownload after interrupted or failed operations.
---
Outside diff comments:
In `@crates/qmrust-core/src/core/model.rs`:
- Around line 568-579: Update Model::samples to validate that the signal
iterator produces exactly one value per entry in values before constructing
Measurement::Series. Reject both shorter and longer signal inputs instead of
relying on zip truncation, while preserving the existing Sample mapping for
matching lengths.
🪄 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: f56868ca-ad8d-42dd-b9fb-03da770385e4
📒 Files selected for processing (17)
ci/datasets.shci/integration_osf.shcrates/qmrust-cli/src/commands.rscrates/qmrust-cli/src/main.rscrates/qmrust-core/src/core/model.rscrates/qmrust-core/src/models/b1_afi/model.rscrates/qmrust-core/src/models/b1_dam/model.rscrates/qmrust-core/src/models/inversion_recovery/model.rscrates/qmrust-core/src/models/mono_t2/model.rscrates/qmrust-core/src/models/mt_ratio/model.rscrates/qmrust-core/src/models/mt_sat/model.rscrates/qmrust-core/src/models/qmt_spgr/adapter.rscrates/qmrust-core/src/models/vfa_t1/model.rsdocs/agents/ADDING-A-MODEL.mddocs/agents/ARCHITECTURE.mddocs/agents/DATA-PIPELINE.mdscripts/make_bids_examples.sh
🚧 Files skipped from review as they are similar to previous changes (9)
- docs/agents/ARCHITECTURE.md
- docs/agents/DATA-PIPELINE.md
- docs/agents/ADDING-A-MODEL.md
- crates/qmrust-core/src/models/mt_ratio/model.rs
- crates/qmrust-core/src/models/b1_afi/model.rs
- crates/qmrust-core/src/models/qmt_spgr/adapter.rs
- crates/qmrust-core/src/models/b1_dam/model.rs
- crates/qmrust-core/src/models/vfa_t1/model.rs
- crates/qmrust-core/src/models/mono_t2/model.rs
Comment on lines
+24
to
+31
| fetch() { | ||
| local name="$1" url="$2" | ||
| if [ ! -d "$DATA/$name" ]; then | ||
| echo "Downloading qMRLab OSF $name dataset..." | ||
| curl -L --fail -o "$DATA/$name.zip" "$url" | ||
| unzip -o -q "$DATA/$name.zip" -d "$DATA/$name" | ||
| fi | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fix fetch()'s all-or-nothing caching.
fetch() treats $DATA/$name existing as proof the archive is fully unpacked. unzip -d creates that directory as part of extraction, before extraction necessarily completes. If unzip is interrupted or fails partway (disk full, corrupt zip, cancelled CI job), the directory exists but is incomplete. Every later call to fetch_all then skips the download, and the failure only surfaces downstream as a fatal, hard-to-diagnose locate() error ("$pattern not found in $name archive"), requiring an operator to manually clear the cache directory.
Make the fetch atomic: only keep the directory if both curl and unzip succeed, and clean up on failure so a retry redownloads.
🛠️ Proposed fix for atomic fetch/unpack
fetch() {
local name="$1" url="$2"
if [ ! -d "$DATA/$name" ]; then
echo "Downloading qMRLab OSF $name dataset..."
- curl -L --fail -o "$DATA/$name.zip" "$url"
- unzip -o -q "$DATA/$name.zip" -d "$DATA/$name"
+ if ! curl -L --fail -o "$DATA/$name.zip" "$url" \
+ || ! unzip -o -q "$DATA/$name.zip" -d "$DATA/$name"; then
+ rm -rf "$DATA/$name" "$DATA/$name.zip"
+ echo "Failed to fetch/unpack $name dataset; retry will redownload" >&2
+ exit 1
+ fi
fi
}🤖 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/datasets.sh` around lines 24 - 31, Update fetch() so downloading and
extraction are atomic: track the archive and destination created for $name, and
on any curl or unzip failure remove the partially extracted directory and
archive before returning failure. Keep the existing cache-skip behavior only
when $DATA/$name already represents a successful extraction, allowing later
fetch_all retries to redownload after interrupted or failed operations.
added 9 commits
August 2, 2026 18:18
A protocol row was running to three pieces on one line: the quantity, its symbol in parentheses, and the BIDS mark. The name is what a reader scans for, so it now stands alone and everything qualifying the value sits beneath it. The second line is set in the mono face the values use and tinted with the muted token, so the eye pairs "TI [s]" with the numbers beside it rather than with the name above it. The BIDS mark leads that line: it is a statement about where the value came from, which is what the line is for. A field with no physical dimension has no second line at all. `labels.js` holds the quantity and the symbol apart rather than pre-joined, because splitting a composed label back up with a regex would make the display format the source of truth for the data. A test asserts neither half arrives with the other's text baked in.
`TI [s]` becomes `TI (s)`, and a quantity with no conventional symbol spells its unit out: `T1 Range Start` is followed by `seconds (s)` rather than a bare `s`, which said less than the line it occupied. The line now always ends in a parenthesised abbreviation, so it reads the same way whether or not a symbol precedes it. A test asserts that over the whole table rather than the handful of entries someone thought to list, so a new field cannot reintroduce a bare or bracketed unit. Verified by adding one and watching it fail.
A protocol row is named by the quantity it carries, so it needs no note. An option is a choice about how to fit, and nothing on screen said what choosing it costs: whether dropping the first echo buys accuracy or just spends a point, what a linear fit trades away, which mask `desc-` selects. Every option in the surface now carries one sentence saying what it changes and when a reader would want it, taken from the config's own documentation rather than invented. Acquisition fields deliberately carry none; a note on every row is noise, and that asymmetry is asserted both ways. Rendered as the same `.info` button the panel headings use, which `wireTips` already picks up by delegation, so a row built after startup needs no wiring. Coverage is checked against the catalog's real option surface rather than a hand-listed set, so an option added later shows up as missing. Two further tests keep the text usable in a tooltip: one sentence, ending in a full stop, under 200 characters.
The hover text added in the previous commit was covered by tests that iterated a list written beside them, so an option added in Rust would have failed nothing: the text lives in the playground, and a developer adding a config field has no reason to look there. `option_help.test.mjs` now reads the option surface out of `qmrust catalog`, subtracts each model's own declared acquisition axes, and fails naming any option with no hover. Verified by adding a `brand_new_knob` field to mono_t2 and watching the test fail with `mono_t2: brand_new_knob`, then removing it. The format checks likewise iterate the label tables themselves, via a `labelledPaths()` export, rather than a parallel list. Also sharpens the `offset_term` sentence: reading the fitter rather than the doc comment shows the linear path never sees it and the config rejects the combination outright, so "rejected with the linear fit" is truer than "only". `drop_first_echo` is applied before the fit-type branch and does affect both, which the existing text already reflected.
…er behave The mark now precedes the option name. A column of options has its marks aligned down one edge, which reads as "these are the explainable ones" without being read, and the label text no longer shifts depending on whether one follows it. `cursor: help` is gone. A cursor change announces a control only once the pointer is already on it, and says nothing about whether the hover took. The mark now lights to the accent, grows slightly and picks up a glow, so the feedback is on the thing being aimed at. The glow derives from `--accent` rather than `--glow`, which four of the six themes set to `transparent` as a deliberate "this theme does not bloom". The tooltip that sometimes never appeared was a real bug, not a near miss: the glyph inside the button is its own event target, so moving from the glyph onto the button's padding fired `pointerleave` on the glyph, and the delegated handler read that as leaving the control. The children are no longer hit targets, so enter and leave now describe the button. The tip itself is frosted: translucent, blurred, hairline-bordered, and it fades in. The accent tint stays dominant so `--on-accent` remains the colour it was measured against whatever sits behind it, and the animation is dropped under `prefers-reduced-motion`.
…t is The previous tip was the accent at 88% opacity, which is opaque enough that the blur behind it had nothing to show: it read as a solid box with an expensive filter attached. Measuring the alternative turned up a real defect rather than only a cosmetic one. A tinted fill at any usable alpha lightens over a pale panel, and white copy on the accent drops to 3.73:1 in clinical/light, under the 4.5 floor. That was already true of the 90% tip this replaces; the contrast checker missed it because it measures `--on-accent` against *solid* `--accent`, which is the primary button's situation and no longer the tooltip's. So the surface is now the neutral panel at 62% over a 16px blur, with `--ink` on it: genuinely see-through, and 10.95:1 at worst across every theme and backdrop. The accent stays as the border and the halo, tying the tip to the mark that opened it, and an inset top highlight gives it a lit edge rather than a washed-out one. The checker's comment is corrected to say what its accent pair now guards.
Clicking the lit half did nothing. The comment above it defended that as
deliberate ("each half selects its own mode"), which is the behaviour of two
buttons that happen to be touching, not of the switch this is drawn as. Aiming
at a control and having it ignore you reads as broken, and there is no way to
tell that from actually broken.
Either half now flips the mode. Clicking the unlit one behaves exactly as
before; clicking the lit one no longer swallows the click. The lit half reports
the state, the labels say what a click will do, and `aria-pressed` is dropped
because it described each half as its own toggle.
The rule lives in `nextMode` in themes.js rather than inline in the handler, so
it is one line with one home and a test can assert the property that matters:
it is never a no-op, from either side or from an unset attribute.
The slider now reads "Slide to Disarm Protocol Inputs" and, once swept, "Slide to Arm Protocol Inputs". Disarmed, the Fit button is disabled. Editing an acquisition the sidecars also supply is refused by the core, so the click could only ever end in an error; the button now says so before it is pressed rather than after. `syncFitArmed` is the one place that decides it, and `fit.js` re-enables through it instead of setting `disabled = false` directly, so a fit that finishes cannot hand the button back while the inputs are disarmed.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/playground/app.css (1)
674-692: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an opacity-aware non-text contrast check.
Composite
--mutedat0.55opacity over the effective.groupsurface (--panel-2) and enforce the WCAG 1.4.11 floor of 3:1 for every theme. The dimmed icon currently measures only 1.84–2.95:1, while the checker validates only full-opacity--muted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/playground/app.css` around lines 674 - 692, Update the theme color definitions used by .field-label .info so its 0.55-opacity composite over the effective .group/--panel-2 surface meets the WCAG 1.4.11 minimum 3:1 contrast in every theme. Preserve the existing dimmed appearance where possible, but adjust the relevant muted/icon color or opacity values rather than changing unrelated field-label styling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/playground/labels.js`:
- Around line 38-129: Update the fit_type help lookup used by fieldHelp so its
guidance is model-aware: retain the Mono-T2 log-transform description and
provide VFA-T1-specific Fram sin/tan linearization guidance. Pass the model
identity into fieldHelp or use model-qualified HELP keys, ensuring each model
resolves the correct description instead of the bare fit_type fallback.
In `@docs/playground/recipe.js`:
- Around line 514-525: Update syncFitArmed in recipe.js to derive the Fit
button’s disabled state from app.overrideProtocol, model availability, and the
in-progress fit state. Remove the direct $("fit").disabled assignments in the
model.js flows so all Fit-button state changes use syncFitArmed and cannot
re-enable the button while any disabling condition applies.
---
Nitpick comments:
In `@docs/playground/app.css`:
- Around line 674-692: Update the theme color definitions used by .field-label
.info so its 0.55-opacity composite over the effective .group/--panel-2 surface
meets the WCAG 1.4.11 minimum 3:1 contrast in every theme. Preserve the existing
dimmed appearance where possible, but adjust the relevant muted/icon color or
opacity values rather than changing unrelated field-label styling.
🪄 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: 32f5bdf9-c47e-4894-9d6e-82b970a3c0dd
📒 Files selected for processing (12)
docs/agents/ADDING-A-MODEL.mddocs/playground/app.cssdocs/playground/app.jsdocs/playground/fit.jsdocs/playground/index.htmldocs/playground/labels.jsdocs/playground/recipe.jsdocs/playground/themes.jsscripts/check_theme_contrast.mjsscripts/tests/labels.test.mjsscripts/tests/option_help.test.mjsscripts/tests/theme_resolve.test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/agents/ADDING-A-MODEL.md
Comment on lines
38
to
129
| const BY_KEY = new Map([ | ||
| ["flip_angle", "Flip Angle (FA°)"], | ||
| ["flip_angles", "Flip Angles (FA°)"], | ||
| ["repetition_time", "Repetition Time (TR [s])"], | ||
| ["repetition_times", "Repetition Times (TR [s])"], | ||
| ["echo_time", "Echo Time (TE [s])"], | ||
| ["echo_times", "Echo Times (TE [s])"], | ||
| ["inversion_time", "Inversion Time (TI [s])"], | ||
| ["inversion_times", "Inversion Times (TI [s])"], | ||
| ["saturation_time", "Saturation Time (s)"], | ||
| ["offsets", "Offset Frequencies (Δ [Hz])"], | ||
| ["angles", "Saturation Flip Angles (°)"], | ||
| ["mtdata", "Saturation Angle / Offset (° , Hz)"], | ||
| ["b1_correction_factor", "B1 Correction Factor"], | ||
| ["b1_correction", "B1 Correction Surface"], | ||
| ["export_mtr", "Export MTR"], | ||
| ["fit_type", "Fit Type"], | ||
| ["drop_first_echo", "Drop First Echo"], | ||
| ["offset_term", "Offset Term"], | ||
| ["flip_angle", ["Flip Angle", "FA (°)"]], | ||
| ["flip_angles", ["Flip Angles", "FA (°)"]], | ||
| ["repetition_time", ["Repetition Time", "TR (s)"]], | ||
| ["repetition_times", ["Repetition Times", "TR (s)"]], | ||
| ["echo_time", ["Echo Time", "TE (s)"]], | ||
| ["echo_times", ["Echo Times", "TE (s)"]], | ||
| ["inversion_time", ["Inversion Time", "TI (s)"]], | ||
| ["inversion_times", ["Inversion Times", "TI (s)"]], | ||
| ["saturation_time", ["Saturation Time", "seconds (s)"]], | ||
| ["offsets", ["Offset Frequencies", "Δ (Hz)"]], | ||
| ["angles", ["Saturation Flip Angles", "degrees (°)"]], | ||
| ["mtdata", ["Saturation Angle / Offset", "degrees (°), hertz (Hz)"]], | ||
| ["b1_correction_factor", ["B1 Correction Factor", null]], | ||
| ["b1_correction", ["B1 Correction Surface", null]], | ||
| ["export_mtr", ["Export MTR", null]], | ||
| ["fit_type", ["Fit Type", null]], | ||
| ["drop_first_echo", ["Drop First Echo", null]], | ||
| ["offset_term", ["Offset Term", null]], | ||
| ]); | ||
|
|
||
| // What an option does, for the hover beside its label. | ||
| // | ||
| // Only options carry one. An acquisition field does not: `Echo Times` is the | ||
| // echo times, and a note saying so is noise on every protocol row. An option is | ||
| // a choice about *how* to fit, and nothing on screen says what choosing it | ||
| // costs, so each entry answers that in a sentence: what it changes, and when a | ||
| // reader would want it. | ||
| // | ||
| // Keyed the same way as the labels, exact path before leaf name, so a nested | ||
| // option (`zoom.points`) is described without colliding with a bare `points` | ||
| // elsewhere. | ||
| const HELP = new Map([ | ||
| ["method", "Whether the inversion-recovery data is complex (signed) or " | ||
| + "magnitude. Magnitude data has lost the sign at the null, so the fit " | ||
| + "restores polarity before solving."], | ||
| ["t1_range.start", "Lower end of the T1 grid the search starts from. " | ||
| + "Narrowing it speeds the fit; a true T1 outside it cannot be found."], | ||
| ["t1_range.stop", "Upper end of the T1 grid. A tissue T1 above this is " | ||
| + "clipped to the boundary rather than fitted."], | ||
| ["t1_range.step", "Spacing of the initial T1 grid. The zoom refinement below " | ||
| + "resolves finer than this, so it trades startup cost, not accuracy."], | ||
| ["zoom.iterations", "How many times the search narrows around its best T1. " | ||
| + "Each pass refines the estimate between the two neighbouring grid points."], | ||
| ["zoom.points", "How many T1 values each zoom pass tries. More is finer but " | ||
| + "costs a full grid evaluation per voxel."], | ||
| ["fit_type", "Linear log-transforms the signal and solves in closed form: " | ||
| + "fast, but it weights the noise unevenly. Nonlinear fits the signal " | ||
| + "itself, which is slower and unbiased at low SNR."], | ||
| ["drop_first_echo", "Discard the shortest echo, whose refocusing is " | ||
| + "imperfect and biases T2 low. Costs one point from an already short " | ||
| + "series."], | ||
| ["offset_term", "Fit an additive constant alongside the decay, absorbing " | ||
| + "residual signal that does not decay. Rejected with the linear fit, " | ||
| + "which has no term to put it in."], | ||
| ["b1_correction_factor", "Empirical scaling of the transmit correction " | ||
| + "(Helms 2015). Applied only when the dataset supplies a B1 map; ignored " | ||
| + "otherwise."], | ||
| ["b1_correction", "A calibration surface from `qmrust mtsat-b1`, correcting " | ||
| + "MTsat for transmit inhomogeneity. Without it the plain Helms " | ||
| + "correction is used."], | ||
| ["export_mtr", "Also write an MTR map from the same volumes. Needs the MT " | ||
| + "and PD volumes to share a repetition time, since MTR is their ratio."], | ||
| ["mask.desc", "Which mask in the dataset to fit inside, by its `desc-` " | ||
| + "label. Left blank, any single mask present is used; a dataset with " | ||
| + "none is fitted whole."], | ||
| ["qmt_spgr.model", "Which two-pool approximation to fit. Ramani is a " | ||
| + "closed-form steady-state solution; SledPikeRP integrates the " | ||
| + "saturation pulse."], | ||
| ["qmt_spgr.lineshape", "Absorption lineshape of the bound pool. " | ||
| + "SuperLorentzian is standard for white matter; Gaussian and Lorentzian " | ||
| + "suit other tissue."], | ||
| ["qmt_spgr.read_pulse_alpha", "Flip angle of the imaging readout, which " | ||
| + "saturates the free pool alongside the MT pulse."], | ||
| ["qmt_spgr.pulse.shape", "Envelope of the off-resonance saturation pulse. " | ||
| + "It sets the power deposited at each offset."], | ||
| ["qmt_spgr.pulse.bandwidth", "Bandwidth of that saturation pulse, in hertz."], | ||
| ["qmt_spgr.fitting.st", "Starting values for the six fitted parameters. A " | ||
| + "poor start can settle the optimiser in a local minimum."], | ||
| ["qmt_spgr.fitting.lb", "Lower bounds. A parameter pinned to its bound in " | ||
| + "the output map means the data did not constrain it."], | ||
| ["qmt_spgr.fitting.ub", "Upper bounds, read the same way as the lower ones."], | ||
| ["qmt_spgr.fitting.fx", "Which parameters are held at their starting value " | ||
| + "instead of fitted, one flag each."], | ||
| ["qmt_spgr.fitting.fix_r1f_t2f", "Hold the free-pool R1*T2 product fixed " | ||
| + "rather than fitting T2f, which the data constrains only weakly."], | ||
| ["qmt_spgr.fitting.r1f_t2f", "The value that product is held at when fixed."], | ||
| ["qmt_spgr.fitting.fix_r1r_eq_r1f", "Assume both pools relax at the same " | ||
| + "rate, removing one poorly-determined parameter."], | ||
| ["qmt_spgr.fitting.use_r1map_to_constrain_r1f", "Take R1f from a supplied " | ||
| + "R1 map rather than fitting it, when the dataset provides one."], | ||
| ]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search model configs for reused generic option field names that could collide
# with labels.js HELP entries keyed only by leaf name.
rg -n -B2 -A2 '\bmethod\s*:' crates/qmrust-core/src/models --type rust
rg -n -B2 -A2 '\bfit_type\s*:' crates/qmrust-core/src/models --type rust
rg -n -B2 -A2 '\bdrop_first_echo\s*:|\boffset_term\s*:|\bexport_mtr\s*:' crates/qmrust-core/src/models --type rustRepository: qMRLab/qmrust
Length of output: 19236
🏁 Script executed:
#!/bin/bash
# Inspect help resolution and the colliding option definitions, including their
# model paths and semantics.
printf '%s\n' '--- labels.js resolution ---'
sed -n '130,190p' docs/playground/labels.js
printf '%s\n' '--- fit_type definitions and docs ---'
sed -n '1,45p' crates/qmrust-core/src/models/mono_t2/config.rs
sed -n '1,45p' crates/qmrust-core/src/models/vfa_t1/config.rs
rg -n -B3 -A8 'enum FitType|pub enum FitType|FitType' crates/qmrust-core/src/models/mono_t2 crates/qmrust-core/src/models/vfa_t1
printf '%s\n' '--- playground model/config registration ---'
rg -n -B3 -A8 'fieldHelp|HELP|getHelp|fit_type|method|model' docs/playground --glob '*.js' --glob '*.mjs' | head -240Repository: qMRLab/qmrust
Length of output: 50369
🏁 Script executed:
#!/bin/bash
# Use a small static verifier to enumerate public config fields and compare
# repeated leaf names with the bare HELP keys.
python3 - <<'PY'
from pathlib import Path
import re
help_keys = {
"method", "fit_type", "drop_first_echo", "offset_term", "export_mtr"
}
root = Path("crates/qmrust-core/src/models")
fields = {}
for file in root.glob("*/config.rs"):
model = file.parent.name
text = file.read_text()
for match in re.finditer(r"^\s*pub\s+([A-Za-z_][A-Za-z0-9_]*)\s*:", text, re.M):
key = match.group(1)
if key in help_keys:
fields.setdefault(key, []).append(f"{model}/{file.name}:{text.count(chr(10), 0, match.start()) + 1}")
for key in sorted(help_keys):
print(f"{key}: {fields.get(key, [])}")
PY
printf '%s\n' '--- option semantics ---'
sed -n '1,42p' crates/qmrust-core/src/models/mono_t2/config.rs
sed -n '1,42p' crates/qmrust-core/src/models/vfa_t1/config.rs
rg -n -B4 -A10 'enum FitType|FitType::Linear|FitType::Nonlinear|Fram|Levenberg' crates/qmrust-core/src/models/mono_t2 crates/qmrust-core/src/models/vfa_t1Repository: qMRLab/qmrust
Length of output: 41035
🏁 Script executed:
#!/bin/bash
# Determine whether fieldHelp receives a model-qualified path. Limit output to
# its call sites and the config-tree walker.
rg -n -B8 -A12 'fieldHelp\(' docs/playground --glob '*.js' --glob '*.mjs'
rg -n -B8 -A20 'fieldLabel\(|fieldUnit\(|labelledPaths\(' docs/playground --glob '*.js' --glob '*.mjs'Repository: qMRLab/qmrust
Length of output: 10469
Make fit_type help model-aware. Both mono_t2 and vfa_t1 define this top-level field, so the bare fallback shows Mono-T2 log-transform guidance for VFA-T1, which uses Fram sin/tan linearization. Pass model identity to fieldHelp or use model-qualified paths.
🤖 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/labels.js` around lines 38 - 129, Update the fit_type help
lookup used by fieldHelp so its guidance is model-aware: retain the Mono-T2
log-transform description and provide VFA-T1-specific Fram sin/tan linearization
guidance. Pass the model identity into fieldHelp or use model-qualified HELP
keys, ensuring each model resolves the correct description instead of the bare
fit_type fallback.
Comment on lines
+514
to
+525
| // Disarmed protocol inputs cannot be fitted: the recipe would then state an | ||
| // acquisition the sidecars also supply, which the core refuses. The button is | ||
| // disabled rather than left to fail, so the state is visible before the click. | ||
| export function syncFitArmed() { | ||
| const fit = $("fit"); | ||
| if (!fit) return; | ||
| fit.disabled = Boolean(app.overrideProtocol); | ||
| fit.title = app.overrideProtocol | ||
| ? "Arm the protocol inputs to fit: edited values cannot be fitted against a BIDS dataset" | ||
| : ""; | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find every direct assignment to the Fit button's `disabled` property to confirm
# all of them route through syncFitArmed() rather than bypassing the protocol-input gate.
rg -n '\$\("fit"\)\.disabled|getElementById\("fit"\)\.disabled' docs/playground --type jsRepository: qMRLab/qmrust
Length of output: 325
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- direct Fit-button state assignments ---'
rg -n -C 3 'fit["'\"'\"']?\)?\.disabled|disabled\s*=\s*.*fit|id=["'\"'\"']fit["'\"'\"']|syncFitArmed' docs/playground --glob '*.js' --glob '*.html'
printf '%s\n' '--- fit.js context ---'
sed -n '55,95p' docs/playground/fit.js
printf '%s\n' '--- model.js contexts ---'
sed -n '105,140p' docs/playground/model.js
sed -n '295,325p' docs/playground/model.js
printf '%s\n' '--- syncFitArmed references ---'
rg -n -C 2 'syncFitArmed' docs/playground --glob '*.js'Repository: qMRLab/qmrust
Length of output: 402
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- direct disabled assignments ---'
rg -n -C 3 '\.disabled\s*=' docs/playground --glob '*.js'
printf '%s\n' '--- syncFitArmed references ---'
rg -n -C 2 'syncFitArmed' docs/playground --glob '*.js'
printf '%s\n' '--- fit.js context ---'
sed -n '55,95p' docs/playground/fit.js
printf '%s\n' '--- model.js contexts ---'
sed -n '105,140p' docs/playground/model.js
sed -n '295,325p' docs/playground/model.jsRepository: qMRLab/qmrust
Length of output: 11411
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- model imports and Fit-button writers ---'
sed -n '1,30p' docs/playground/model.js
sed -n '105,135p' docs/playground/model.js
sed -n '300,322p' docs/playground/model.js
printf '%s\n' '--- fit lifecycle and imports ---'
sed -n '1,12p' docs/playground/fit.js
sed -n '68,145p' docs/playground/fit.js
printf '%s\n' '--- all Fit-button references ---'
rg -n -C 2 'fit|Fit' docs/playground --glob '*.js' --glob '*.html' | rg -n 'disabled|syncFitArmed|fitSlice|id="fit"|getElementById|querySelector'
printf '%s\n' '--- overrideProtocol writers and readers ---'
rg -n -C 3 'overrideProtocol' docs/playground --glob '*.js'Repository: qMRLab/qmrust
Length of output: 50370
Centralize Fit-button state in syncFitArmed.
docs/playground/model.js directly sets $("fit").disabled at lines 128 and 314. These assignments can re-enable Fit while app.overrideProtocol is true. Include model availability and the in-progress fit state in the centralized logic before removing the direct assignments.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/playground/recipe.js` around lines 514 - 525, Update syncFitArmed in
recipe.js to derive the Fit button’s disabled state from app.overrideProtocol,
model availability, and the in-progress fit state. Remove the direct
$("fit").disabled assignments in the model.js flows so all Fit-button state
changes use syncFitArmed and cannot re-enable the button while any disabling
condition applies.
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.
Lands the two commits that were merged but never reached
main, thedocumentation they made necessary, and the violations that documentation then
exposed.
Why these commits were stranded
#29 was stacked on
feat/fieldmap-models, and #28 merged eleven secondsbefore it, so #29 landed on a branch
mainhad already taken a snapshot of.Both PRs report MERGED, correctly, but only one reached
main.Contents
Already reviewed on #29:
c872e88collapses 31 copy-pasted per-model tests into 6 shared propertiesin
tests/properties.rs, guarded by what each model declares rather than byits name.
e18be29addsSeriesAxisandmodel_entry_points!, removing 435 linesacross the model modules.
The rules those refactors were applying, written down:
e2ad215makes DRY, AHA and the Rule of Three explicit in CLAUDE.md, andresyncs the agent docs. Not cosmetic:
ADDING-A-MODEL.mdstill told a newmodel to hand-write the four registry entry points and its own copies of
contracts now asserted once over the registry, so following it would have
reintroduced precisely the duplication removed above. It also omitted
SeriesAxis, describedCategoryas flat, presented the BIDS datatype as amodel decision, and was missing
fit_block/sim_required_aux. Threederivative paths still hardcoded
anat/.What holding the code against those rules then found:
0103863removesBidsSpec.entities, a second declaration of which entitiesindex a model's suffix that nothing ever read: the grouping grammar is what
assembles a collection, so a model could have said
Echowhile the grammarsaid
flipwith nothing to notice. Also dropsif cfg.model == "mt_sat"from both fit paths (the guard was redundant, the function already no-ops
without the key) and corrects
bidsify --help, which still claimed thesubcommand was for two specific models.
a664fbdgives the dataset provenance one home.ci/integration_osf.shandscripts/make_bids_examples.sheach stated every archive's pinned version,its internal layout and its
bidsifyinvocation. Bump?version=in one andnot the other and both stay green while CI validates against data the reader
never sees.
Verification
Re-checked against current
main, which moved after #29 was reviewed (#27landed on top). Merging
mainin touches only #27's four playground files,with no conflicts.
Behaviour is unchanged, and checked rather than assumed at every step, because
a fit shifted by 1e-15 would pass every test:
Please do not delete
feat/fieldmap-modelsuntil this merges; it is the onlyplace these commits exist.
https://claude.ai/code/session_01JDh9nrWyw1ju5TZ8rLp4dj
Summary by CodeRabbit
anat/andfmap/.b1_correction.fitvalueshandling across applicable recipes.