Generalize model operations: - #5
Merged
Merged
Conversation
added 19 commits
July 21, 2026 17:09
…rouping style Claude-Session: https://claude.ai/code/session_01FGgDgPiv84XZBxVJRcwacy
…tale qMT/aux claims Claude-Session: https://claude.ai/code/session_01FGgDgPiv84XZBxVJRcwacy
…ify and ProtocolSource::Mat Claude-Session: https://claude.ai/code/session_01FGgDgPiv84XZBxVJRcwacy
…d ProtocolSource + describe/dump registry Claude-Session: https://claude.ai/code/session_01FGgDgPiv84XZBxVJRcwacy
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR centralizes model configuration and protocol handling, adds model-driven BIDS writing and grouping overrides, removes MAT inversion-time overrides, introduces recipe directories, and updates BIDS resolution, documentation, tests, and integration commands. ChangesModel-driven configuration and BIDS execution
BIDS grouping resolution
Recipes and documentation migration
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
CLAUDE.md (2)
111-111: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSpecify the fenced-block language.
markdownlint-cli2reports MD040 on this fence. Change the opening fence to```text(or another appropriate language).🤖 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 `@CLAUDE.md` at line 111, Specify a language on the fenced code block in CLAUDE.md by changing its opening fence to use text (or another suitable language identifier), while leaving the block contents unchanged.Source: Linters/SAST tools
119-125: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the complete ModelConfig contract.
The example declares
ModelConfig: DeserializeOwned, butcrates/qmrust-core/src/core/model.rsalso requiresserde::Serialize + Default. Make this snippet match the actual trait so contributors do not omit required derives or implementations.🤖 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 `@CLAUDE.md` around lines 119 - 125, Update the ModelConfig contract in the surrounding documentation to include serde::Serialize and Default alongside DeserializeOwned, matching the bounds defined in the model configuration API. Ensure the snippet clearly communicates that implementations must provide all required derives or implementations.
🧹 Nitpick comments (4)
crates/qmrust-cli/src/commands.rs (1)
1159-1169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim the historical-narrative sentence from this test doc comment.
"...so this would catch it where the old (grouping-content-insensitive) version of this test could not" narrates the test's prior state rather than describing what the test verifies now.
✏️ Proposed trim
/// A `--grouping` manifest overriding the built-in default is actually /// threaded into collection resolution — not read and then ignored in /// favor of `rust_bids::default_config()`. Proven by contrast: the same /// synthetic IRT1 dataset that resolves and fits under the default /// grouping (`run_fit_bids_recovers_t1_from_a_synthetic_dataset`) fails - /// to resolve any `IRT1` collection under a custom manifest that omits - /// the `IRT1` set entirely. A regression that silently discarded the - /// custom grouping and fell back to the default would still resolve and - /// fit here, so this would catch it where the old (grouping-content- - /// insensitive) version of this test could not. + /// to resolve any `IRT1` collection under a custom manifest that omits + /// the `IRT1` set entirely. A regression that silently discarded the + /// custom grouping and fell back to the default would still resolve and + /// fit here, so this test would catch it. #[test]As per coding guidelines, "Comments and docstrings must explain what code does and the invariant or contract behind it, not author decisions, alternatives, historical narratives, reviewer asides, 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/qmrust-cli/src/commands.rs` around lines 1159 - 1169, Trim the doc comment above the test by removing the historical sentence beginning with “A regression that silently discarded...” and ending with “the old (grouping-content-insensitive) version of this test could not.” Keep the remaining description focused on verifying that a custom grouping manifest is used during collection resolution.Source: Coding guidelines
crates/qmrust-cli/src/io/mat.rs (2)
70-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the redundant/self-correcting comment lines in the 2D reshape branch.
Lines 71-73 restate and second-guess themselves ("But more commonly...", "Actually MATLAB 2D means...") before lines 75-77 give the actual, clear explanation. This reads as in-progress author reasoning rather than an invariant description.
🧹 Proposed cleanup
2 => { - // (rows, cols) in MATLAB = (spatial, n_vol) — treat as (rows, 1, 1, cols) - // But more commonly the source data has shape (x, y, n_vol) stored as 2D - // Actually MATLAB 2D means (x*y, n_vol) or (x, n_vol) let (nrows, ncols) = (data_size[0], data_size[1]); // Treat as (nrows, 1, 1, ncols) — each row is a voxel, cols are volumes // MATLAB column-major: data is stored column by column // Column j values are at indices j*nrows..(j+1)*nrows let mut arr = Array4::<f64>::zeros((nrows, 1, 1, ncols));As per coding guidelines, "Comments and docstrings must explain what code does and the invariant or contract behind it, not author decisions, alternatives, historical narratives, reviewer asides, 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/qmrust-cli/src/io/mat.rs` around lines 70 - 78, Remove the self-correcting comment lines in the 2D reshape branch before the nrows/ncols assignment, specifically the “But more commonly...” and “Actually MATLAB 2D means...” remarks. Keep the concise invariant comments explaining the resulting (nrows, 1, 1, ncols) shape and MATLAB column-major storage.Source: Coding guidelines
44-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffHardcoded model-specific variable names sit at odds with the registry-driven "add a model in one place" goal.
read_mat_filestill special-cases the exact qMRLab variable names (IRdata/IRData/MTdata/MTData) for the two known models. A third model with a different.matnaming convention would require editing this file too, contradicting the PR's stated goal of "module + trait implementations + one registry entry."
Not a blocker today (only 2 models registered), but worth tracking if a third model with a different export convention lands.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/qmrust-cli/src/io/mat.rs` around lines 44 - 63, The read_mat_file measurement-array lookup is hardcoded to qMRLab-specific names, preventing registry-only model additions. Replace the IRdata/IRData/MTdata/MTData chain with a registry- or model-provided naming strategy, while preserving the existing missing-array context and available-name diagnostics.crates/qmrust-core/src/models/qmt_spgr/sf.rs (1)
8-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove
matfileto[dev-dependencies]. Its only remaining uses are under#[cfg(test)]incrates/qmrust-core/src/models/qmt_spgr/sf.rs, so it no longer needs to be a regularqmrust-coredependency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/qmrust-core/src/models/qmt_spgr/sf.rs` around lines 8 - 12, Move the matfile crate entry from qmrust-core’s regular dependencies to its dev-dependencies, preserving the test-only imports in sf.rs and ensuring test builds continue to resolve MatFile.
🤖 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/inversion_recovery/fit.rs`:
- Around line 209-210: Update the comment immediately above extract_params to
document its contract: state the parameter quantities it computes and returns,
and specify that ind indexes valid entries, n is within the available
sample/data range, and rho_norm_vec[ind] is nonzero before the division. Replace
the argument-grouping description while keeping the existing argument names and
function behavior unchanged.
In `@crates/rust-bids/src/config.rs`:
- Around line 111-119: Update the named-set constraints construction for
EntityConstraints so each key is canonicalized with full_key before filtering
out description. Ensure both desc and description are excluded after
normalization, while preserving value prefix handling. Add a regression test
covering a named set containing both short and full description keys.
In `@crates/rust-bids/src/entities.rs`:
- Around line 7-15: Update entity_value_cmp in
crates/rust-bids/src/entities.rs:7-15 to establish an explicit ordering between
numeric and non-numeric values, then compare numerically within the numeric
class and lexically within the non-numeric class; ensure the related entity
ordering at crates/rust-bids/src/entities.rs:153-168 and sequential sort usage
at crates/rust-bids/src/resolve.rs:83-88 use this total comparator without
separate changes unless required.
In `@docs/agents/ADDING-A-MODEL.md`:
- Around line 119-125: Update the ModelConfig trait declaration to include
serde::Serialize and Default alongside DeserializeOwned, matching the complete
supertrait contract required by model implementations.
In `@docs/agents/ARCHITECTURE.md`:
- Around line 345-347: Update the ingest_protocol worked example to remove the
`.mat` reference, stating only that BIDS sidecars supply InversionTime values.
Keep the example aligned with the documented contract that MAT inputs provide no
Protocol and acquisition parameters remain in --config.
In `@docs/getting-started.md`:
- Around line 23-24: Update the recipe description near the getting-started
documentation to reference only recipes/non-bids/ as fully explicit, and clarify
that BIDS recipes obtain acquisition protocol values from sidecars. Preserve the
existing self-documenting explanation for the non-BIDS examples without implying
it applies to all recipes.
- Line 63: Update the BIDS output path documentation near the
`<out>/<subject>[/<session>]/<map>.nii.gz` example to include the required
`qmrust` directory: `<out>/qmrust/<subject>[/<session>]/<map>.nii.gz`. Keep the
remaining path structure unchanged.
In `@recipes/bids/irt1_config.yaml`:
- Around line 3-4: Update the comment in the IR recipe to state only the
parameters actually declared and ingested by the model’s protocol_schema(),
removing RepetitionTime unless the schema and related fixtures are explicitly
updated to support it.
In `@scripts/make_bids_examples.sh`:
- Line 70: The BIDS fitting examples must use the BIDS-specific qMT recipe while
preserving the non-BIDS recipe for bidsify. In scripts/make_bids_examples.sh,
keep QMT_CONFIG for bidsify, add a BIDS qMT configuration variable, and use it
for the qMT fit --bids-dir invocation; in docs/getting-started.md at line 139,
change the example to recipes/bids/qmt_config_ramani.yaml.
---
Outside diff comments:
In `@CLAUDE.md`:
- Line 111: Specify a language on the fenced code block in CLAUDE.md by changing
its opening fence to use text (or another suitable language identifier), while
leaving the block contents unchanged.
- Around line 119-125: Update the ModelConfig contract in the surrounding
documentation to include serde::Serialize and Default alongside
DeserializeOwned, matching the bounds defined in the model configuration API.
Ensure the snippet clearly communicates that implementations must provide all
required derives or implementations.
---
Nitpick comments:
In `@crates/qmrust-cli/src/commands.rs`:
- Around line 1159-1169: Trim the doc comment above the test by removing the
historical sentence beginning with “A regression that silently discarded...” and
ending with “the old (grouping-content-insensitive) version of this test could
not.” Keep the remaining description focused on verifying that a custom grouping
manifest is used during collection resolution.
In `@crates/qmrust-cli/src/io/mat.rs`:
- Around line 70-78: Remove the self-correcting comment lines in the 2D reshape
branch before the nrows/ncols assignment, specifically the “But more
commonly...” and “Actually MATLAB 2D means...” remarks. Keep the concise
invariant comments explaining the resulting (nrows, 1, 1, ncols) shape and
MATLAB column-major storage.
- Around line 44-63: The read_mat_file measurement-array lookup is hardcoded to
qMRLab-specific names, preventing registry-only model additions. Replace the
IRdata/IRData/MTdata/MTData chain with a registry- or model-provided naming
strategy, while preserving the existing missing-array context and available-name
diagnostics.
In `@crates/qmrust-core/src/models/qmt_spgr/sf.rs`:
- Around line 8-12: Move the matfile crate entry from qmrust-core’s regular
dependencies to its dev-dependencies, preserving the test-only imports in sf.rs
and ensuring test builds continue to resolve MatFile.
🪄 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: 344ecaa6-967c-43ff-bba4-f94ec7c91a2d
📒 Files selected for processing (43)
CLAUDE.mdREADME.mdci/integration_osf.shcrates/qmrust-cli/src/bidsify.rscrates/qmrust-cli/src/commands.rscrates/qmrust-cli/src/io/mat.rscrates/qmrust-cli/src/main.rscrates/qmrust-core/src/core/model.rscrates/qmrust-core/src/engine.rscrates/qmrust-core/src/lib.rscrates/qmrust-core/src/models/inversion_recovery/config.rscrates/qmrust-core/src/models/inversion_recovery/fit.rscrates/qmrust-core/src/models/inversion_recovery/mod.rscrates/qmrust-core/src/models/inversion_recovery/model.rscrates/qmrust-core/src/models/qmt_spgr/adapter.rscrates/qmrust-core/src/models/qmt_spgr/config.rscrates/qmrust-core/src/models/qmt_spgr/mod.rscrates/qmrust-core/src/models/qmt_spgr/pulse.rscrates/qmrust-core/src/models/qmt_spgr/sf.rscrates/qmrust-core/src/protocol.rscrates/qmrust-core/src/registry.rscrates/qmrust-core/src/sim/mod.rscrates/qmrust-core/src/sim/noise.rscrates/qmrust-wasm/README.mdcrates/rust-bids/src/config.rscrates/rust-bids/src/default_grouping.yamlcrates/rust-bids/src/entities.rscrates/rust-bids/src/resolve.rsdocs/agents/ADDING-A-MODEL.mddocs/agents/ARCHITECTURE.mddocs/agents/DATA-PIPELINE.mddocs/bids.mddocs/getting-started.mddocs/models.mdrecipes/README.mdrecipes/bids/irt1_config.yamlrecipes/bids/qmt_config_ramani.yamlrecipes/bids/qmt_config_sledpikerp.yamlrecipes/non-bids/irt1_config.yamlrecipes/non-bids/qmt_config_ramani.yamlrecipes/non-bids/qmt_config_sledpikerp.yamlrecipes/sim/qmt_sim_ramani.yamlscripts/make_bids_examples.sh
💤 Files with no reviewable changes (4)
- recipes/non-bids/qmt_config_ramani.yaml
- crates/qmrust-core/src/sim/noise.rs
- recipes/non-bids/qmt_config_sledpikerp.yaml
- crates/qmrust-core/src/protocol.rs
Comment on lines
+7
to
+15
| /// Compare two BIDS entity *values*. Index entities may be zero-padded and the | ||
| /// padding is not semantic, so compare numerically when both parse as integers; | ||
| /// otherwise compare lexically (string labels like `off`, `brain`). | ||
| pub(crate) fn entity_value_cmp(a: &str, b: &str) -> std::cmp::Ordering { | ||
| match (a.parse::<i64>(), b.parse::<i64>()) { | ||
| (Ok(x), Ok(y)) => x.cmp(&y), | ||
| _ => a.cmp(b), | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
def cmp(a, b):
try:
ax = int(a)
except ValueError:
ax = None
try:
bx = int(b)
except ValueError:
bx = None
if ax is not None and bx is not None:
return (ax > bx) - (ax < bx)
return (a > b) - (a < b)
assert cmp("2", "10") < 0
assert cmp("10", "1a") < 0
assert cmp("1a", "2") < 0
print("non-transitive comparator counterexample reproduced")
PYRepository: qMRLab/qmrust
Length of output: 203
Make entity ordering total before using it in sequential sorts.
entity_value_cmp is not a total order for mixed numeric/string values ("2" < "10" < "1a" < "2"), so sort_by in crates/rust-bids/src/resolve.rs can see an inconsistent comparator. Define an explicit order between numeric and non-numeric values first, then compare within each class.
📍 Affects 2 files
crates/rust-bids/src/entities.rs#L7-L15(this comment)crates/rust-bids/src/entities.rs#L153-L168crates/rust-bids/src/resolve.rs#L83-L88
🤖 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/entities.rs` around lines 7 - 15, Update
entity_value_cmp in crates/rust-bids/src/entities.rs:7-15 to establish an
explicit ordering between numeric and non-numeric values, then compare
numerically within the numeric class and lexically within the non-numeric class;
ensure the related entity ordering at crates/rust-bids/src/entities.rs:153-168
and sequential sort usage at crates/rust-bids/src/resolve.rs:83-88 use this
total comparator without separate changes unless required.
Comment on lines
+345
to
+347
| fn ingest_protocol(&mut self, proto: &Protocol) -> Result<()> { | ||
| // BIDS sidecars (or a .mat) supply the InversionTimes here. | ||
| if !proto.volumes.is_empty() { /* pull InversionTime values into self.inversion_times */ } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove .mat as a Protocol source from the worked example.
docs/agents/DATA-PIPELINE.md documents that .mat inputs provide no Protocol; acquisition parameters remain in --config. Saying “BIDS sidecars (or a .mat)” supplies inversion times contradicts that contract and may reintroduce MAT protocol ingestion.
Proposed documentation fix
- // BIDS sidecars (or a .mat) supply the InversionTimes here.
+ // BIDS sidecars supply InversionTimes here; non-BIDS builds pass an
+ // empty Protocol and keep acquisition values from the config.As per coding guidelines, docs/agents/ARCHITECTURE.md must stay synchronized with the current system design.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn ingest_protocol(&mut self, proto: &Protocol) -> Result<()> { | |
| // BIDS sidecars (or a .mat) supply the InversionTimes here. | |
| if !proto.volumes.is_empty() { /* pull InversionTime values into self.inversion_times */ } | |
| fn ingest_protocol(&mut self, proto: &Protocol) -> Result<()> { | |
| // BIDS sidecars supply InversionTimes here; non-BIDS builds pass an | |
| // empty Protocol and keep acquisition values from the config. | |
| if !proto.volumes.is_empty() { /* pull InversionTime values into self.inversion_times */ } |
🤖 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 345 - 347, Update the
ingest_protocol worked example to remove the `.mat` reference, stating only that
BIDS sidecars supply InversionTime values. Keep the example aligned with the
documented contract that MAT inputs provide no Protocol and acquisition
parameters remain in --config.
Source: Coding guidelines
added 2 commits
July 21, 2026 23:58
…dev-only; comment/contract cleanups Claude-Session: https://claude.ai/code/session_01FGgDgPiv84XZBxVJRcwacy
…ced protocol, bids qMT recipe in examples Claude-Session: https://claude.ai/code/session_01FGgDgPiv84XZBxVJRcwacy
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-core/Cargo.toml`:
- Line 26: Move the matfile dependency back from dev-dependencies to the normal
dependencies section in crates/qmrust-core/Cargo.toml, preserving availability
for the non-test code in the qmt_spgr::sf module.
🪄 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: ce933a2d-9e00-4eed-9fe8-137d11c6b982
📒 Files selected for processing (12)
README.mdcrates/qmrust-cli/src/commands.rscrates/qmrust-cli/src/io/mat.rscrates/qmrust-core/Cargo.tomlcrates/qmrust-core/src/models/inversion_recovery/fit.rscrates/rust-bids/src/config.rscrates/rust-bids/src/entities.rsdocs/agents/ADDING-A-MODEL.mddocs/agents/ARCHITECTURE.mddocs/getting-started.mdrecipes/bids/irt1_config.yamlscripts/make_bids_examples.sh
🚧 Files skipped from review as they are similar to previous changes (10)
- recipes/bids/irt1_config.yaml
- crates/qmrust-core/src/models/inversion_recovery/fit.rs
- scripts/make_bids_examples.sh
- crates/rust-bids/src/entities.rs
- crates/rust-bids/src/config.rs
- docs/agents/ADDING-A-MODEL.md
- crates/qmrust-cli/src/io/mat.rs
- docs/getting-started.md
- docs/agents/ARCHITECTURE.md
- crates/qmrust-cli/src/commands.rs
| plotters.workspace = true | ||
|
|
||
| [target.'cfg(not(target_arch = "wasm32"))'.dependencies] | ||
| [target.'cfg(not(target_arch = "wasm32"))'.dev-dependencies] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n --glob '*.rs' '\bmatfile\b' crates/qmrust-core || true
cargo test --workspace
cargo fmt --all --check
cargo clippy --workspace --all-targets -- -D warningsRepository: qMRLab/qmrust
Length of output: 783
Keep matfile as a normal dependency crates/qmrust-core/Cargo.toml:26
crates/qmrust-core/src/models/qmt_spgr/sf.rs imports matfile, so moving it to dev-dependencies will break non-test qmrust-core builds.
🤖 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/Cargo.toml` at line 26, Move the matfile dependency back
from dev-dependencies to the normal dependencies section in
crates/qmrust-core/Cargo.toml, preserving availability for the non-test code in
the qmt_spgr::sf module.
Source: Coding guidelines
added 3 commits
July 22, 2026 10:55
… block to bids qMT recipes Claude-Session: https://claude.ai/code/session_01FGgDgPiv84XZBxVJRcwacy
…delEntry/output-path refs Claude-Session: https://claude.ai/code/session_01FGgDgPiv84XZBxVJRcwacy
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 `@CLAUDE.md`:
- Around line 35-40: Update the “Behaviour-preserving refactors do not change
fitting results” section in CLAUDE.md to remove the unresolved TODO and either
provide the actual snapshot/golden-output regression command or describe the
manual verification procedure without any TODO/task reference. Preserve the
requirement that fitting-output differences are treated as regressions.
🪄 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: fbd02433-cf89-49ec-b840-11a012133061
📒 Files selected for processing (18)
CLAUDE.mdREADME.mdcrates/qmrust-cli/src/bidsify.rscrates/qmrust-cli/src/commands.rscrates/qmrust-core/src/config.rscrates/qmrust-core/src/models/qmt_spgr/config.rscrates/qmrust-core/src/models/qmt_spgr/fit.rscrates/qmrust-core/src/quad.rscrates/rust-bids/src/entities.rscrates/rust-bids/src/resolve.rscrates/rust-bids/src/scan.rsdocs/agents/ARCHITECTURE.mddocs/agents/DATA-PIPELINE.mddocs/bids.mddocs/browser.mddocs/getting-started.mddocs/models.mdrecipes/non-bids/qmt_config_ramani.yaml
🚧 Files skipped from review as they are similar to previous changes (10)
- recipes/non-bids/qmt_config_ramani.yaml
- crates/rust-bids/src/entities.rs
- docs/bids.md
- docs/agents/DATA-PIPELINE.md
- crates/qmrust-core/src/models/qmt_spgr/config.rs
- README.md
- docs/getting-started.md
- docs/agents/ARCHITECTURE.md
- crates/qmrust-cli/src/bidsify.rs
- crates/qmrust-cli/src/commands.rs
This was referenced Jul 22, 2026
Merged
This was referenced Aug 1, 2026
Merged
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.
Description:
Makes the model contribution surface fully generic, so adding a qMRI model is a module + trait impls + one registry line — no shell edits. Also folds in the doc/dead-code fixes and the recipes/ reorganization done along the way.
Core change — one seam per operation, dispatched via the registry:
Also in this branch:
Notes for reviewers:
Summary by CodeRabbit
--groupingmanifest support for BIDS fitting.describeand configdump.fit_voxel/forward/fit_volumenow use JSON payloads (measurement + volume IDs).recipes/layout; updated BIDS grouping and wasm usage docs..matinversion times); non-BIDS.matinversion-time overriding is removed.