Add porting-qmrlab-models skill - #6
Conversation
…dsify, grouping step) Claude-Session: https://claude.ai/code/session_01PFNmaQ7VyHWoReV6a5gsUg
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe pull request documents a phased qMRLab model-porting workflow, adds model scaffolding and self-testing scripts, introduces simulation-specific auxiliary-input validation, and updates Model/BIDS integration guidance. ChangesPorting workflow and simulation contracts
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
crates/qmrust-core/src/sim/model.rs (1)
95-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest both branches of the R1map requirement.
Add an explicit
use_r1map_to_constrain_r1f: truecase and afalsecase that accepts a missingsim.r1; the current test relies on the default and only protects the enabled branch.🤖 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/sim/model.rs` around lines 95 - 107, The test sim_enforces_model_declared_aux currently covers only the default enabled R1map requirement. Extend it with explicit use_r1map_to_constrain_r1f: true and false configurations, asserting the true case rejects missing sim.r1 and accepts it when supplied, while the false case accepts a missing sim.r1.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/porting-qmrlab-models/references/validation.md:
- Around line 69-71: The Tier 3 validation definition must require a direct
numerical comparison against qMRLab FitResults, not only OSF fitting and
BIDS-vs-.mat round-trip parity. Update the Tier 3 guidance around the OSF and
qMRust-cli validation references to specify the comparison and an explicit
tolerance; otherwise reclassify those checks as Tier 2 or prerequisites.
In @.claude/skills/porting-qmrlab-models/scaffold_model.sh:
- Around line 45-56: Update scaffold_model.sh to validate that NAME/SUFFIX are
absent from both the registry and grouping file before the perl insertion or
grouping append occurs. If either identity already exists, print an error and
exit nonzero before mutating either file; preserve the existing creation flow
for new entries.
In @.claude/skills/porting-qmrlab-models/SKILL.md:
- Around line 44-46: Update the scaffold command in
`.claude/skills/porting-qmrlab-models/SKILL.md` lines 44-46 and
`.claude/skills/porting-qmrlab-models/references/translation-patterns.md` lines
11-13 to use the repository-relative script path
`.claude/skills/porting-qmrlab-models/scaffold_model.sh`, so both instructions
work when run from the repository root.
In @.claude/skills/porting-qmrlab-models/test_scaffold.sh:
- Around line 7-18: Update the test scaffold setup and cleanup around NAME,
cleanup(), and the EXIT trap to preserve the worktree: refuse to run when the
scaffold_probe directory already exists, and snapshot the affected registry,
module-list, and grouping files before changes, restoring those snapshots during
cleanup instead of using git checkout. Ensure cleanup does not remove any
pre-existing probe directory.
In `@crates/qmrust-core/src/sim/model.rs`:
- Around line 15-29: Make validate_sim_inputs and sim_aux support all logical
names accepted by Model::sim_required_aux, ensuring model-declared auxiliary
inputs can be supplied through SimConfig. If the simulation configuration cannot
represent arbitrary names, replace the generic contract with explicit validation
that rejects unsupported names rather than reporting them as missing.
---
Nitpick comments:
In `@crates/qmrust-core/src/sim/model.rs`:
- Around line 95-107: The test sim_enforces_model_declared_aux currently covers
only the default enabled R1map requirement. Extend it with explicit
use_r1map_to_constrain_r1f: true and false configurations, asserting the true
case rejects missing sim.r1 and accepts it when supplied, while the false case
accepts a missing sim.r1.
🪄 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: 8b27be85-c39b-4549-ae56-ef715a336b78
📒 Files selected for processing (15)
.claude/skills/porting-qmrlab-models/SKILL.md.claude/skills/porting-qmrlab-models/references/optional-inputs.md.claude/skills/porting-qmrlab-models/references/reading-qmrlab.md.claude/skills/porting-qmrlab-models/references/translation-patterns.md.claude/skills/porting-qmrlab-models/references/validation.md.claude/skills/porting-qmrlab-models/scaffold_model.sh.claude/skills/porting-qmrlab-models/test_scaffold.shcrates/qmrust-core/src/config.rscrates/qmrust-core/src/core/model.rscrates/qmrust-core/src/models/qmt_spgr/adapter.rscrates/qmrust-core/src/sim/mod.rscrates/qmrust-core/src/sim/model.rsdocs/agents/ADDING-A-MODEL.mddocs/agents/ARCHITECTURE.mddocs/agents/DATA-PIPELINE.md
…ds, script paths, sim test) Claude-Session: https://claude.ai/code/session_01PFNmaQ7VyHWoReV6a5gsUg
Summary by CodeRabbit