Collapse the repeated single-axis series into one abstraction - #29
Merged
Conversation
added 2 commits
August 2, 2026 13:54
…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.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
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.
Stacked on #28. Base is
feat/fieldmap-models, so the diff here is only therefactor; review #28 first, and GitHub will retarget this to
mainwhen thatmerges.
Two kinds of repetition across the model modules, both replaced by one shared
thing.
The single-axis series
Five models are the same shape with a different string: a
Seriesindexed byone per-volume protocol key (
InversionTime,EchoTime,FlipAngle,RepetitionTimeExcitation). Each reimplemented four things, differing only inthat key:
measurement()declares,forwardemits,fitassembles back out,ingest_protocolreads from resolved sidecars.SeriesAxisstates the key once and provides all four.The third is why this is worth doing rather than merely tidy. An assembly that
reads by position instead of by identity pairs every value with the wrong
protocol row, and produces a plausible wrong map rather than an error. It was
hand-written five times, each with its own hand-written panic message.
qmt_spgrkeeps its own rows: it has two axes (saturation angle and offset),so the abstraction does not apply. The
Namedmodels have no axis at all.Neither is excluded by name; they simply have nothing to state.
The registry entry points
Every model ended with the same four one-line delegations plus the same four
doc comments: 32 functions with nothing model-specific to get right or wrong.
They are now
model_entry_points!(Config). The registry still stores plainfunction pointers, so nothing it depends on changed.
Size
Behaviour is unchanged, and checked rather than assumed
The suite passing is not sufficient evidence here: a refactor that shifted a
fit by 1e-15 would pass every test. So every example dataset was fitted before
the first change and again after the last, and the outputs compared by hash:
Also 405 tests,
cargo fmt --check,clippy -D warningsclean, and theqmrust-corewasm purity build.https://claude.ai/code/session_01JDh9nrWyw1ju5TZ8rLp4dj