[ENH] Fix registration + use mask apply in tractoflow - #369
Conversation
gagnonanthony
left a comment
There was a problem hiding this comment.
Great job! I left a few comments. Also, I'm trying to figure out what someone would expect as output when supplying images with their associated masks. I would assume they don't really want the masked registered images, but probably the original images warped using the mask, no? This means we should probably add an antsApplyTransforms at the end to warp the moving original (non-masked) image. This is just an opinion and I'm not a power user of this subworkflow, so feel free to chip in!
AlexVCaron
left a comment
There was a problem hiding this comment.
Sorry, I missed your message on the merged PR. I was not advocating for prior masking as a replacement to feeding the masks to the registration. As @gdevenyi said, neither one or the other has proven stable really, and both have their use-cases.
I'd rather have both options (including their combination). versaFlow is an example of this. Its registration chain from T1 to DWI needs to configure antsRegistration under all 3 cases to work correctly (and more, but let's not get into that).
|
Ok I stop coding until we agree on the strategy here is the link to what @gdevenyi said: #367 (comment). I think to add an extra layer of masking within or before becomes complicated but maybe I'm getting lazy. |
I think the current implementation is okay for now, we can extend the conversation at the SIG on Thursday and decide on the matter. I'm not against complexity, registration is an unstable and complex workflow. Aside healthy adult human brains, it's non-trivial and requires deep fine-tuning. I'm happy to put my hand in the fryer on this one and contribute to the implementation, as I've done much of what I ask for in versaFlow. |
|
I think the safer thing to do here is to add a "mask extraction" option in each of the individual registration tools so that we only have to pass around the full unmasked images. My registration tool already has this as an option internally (--mask-extract) so its easy for me to implement. |
|
@gdevenyi, @AlexVCaron , @gagnonanthony |
| out_ref_warped_masked = out_ref_warped | ||
| .join(ch_fixed_mask) | ||
| .filter{ _meta, _warped, mask -> mask } | ||
| .map{ meta, warped, _mask -> [meta, warped] } | ||
|
|
There was a problem hiding this comment.
Audit: Invalid for the current code. For the ANTs path, out_ref_warped is populated from REGISTRATION_ANTS.out.fixed_warped (*_warped_reference.nii.gz) before out_ref_warped_masked captures it — the subworkflow test snapshot confirms reference_warped_masked emits test_warped_reference.nii.gz for the SyNQuick test (which has a fixed mask). Same for SynthMorph (REGISTRATION_SYNTHMORPH.out.fixed_warped). The channel is not empty at the point out_ref_warped_masked is derived.
| emit: | ||
| image_warped = out_image_warped // channel: [ val(meta), image ] | ||
| reference_warped = out_ref_warped // channel: [ val(meta), ref ] | ||
| image_warped_masked = out_image_warped_masked // channel: [ val(meta), image ] | ||
| reference_warped_masked = out_ref_warped_masked // channel: [ val(meta), ref ] |
There was a problem hiding this comment.
Audit: Still valid at HEAD (84f37ce). meta.yml does not document image_warped_masked or reference_warped_masked outputs, and reference_warped still states "ONLY PROVIDED BY REGISTRATION_EASYREG" despite now being produced by all registration paths (ANTS, SynthMorph via WARP_IMAGE_TO_MOVING).
| if ( ( options.masking_strategy == "apriori" || options.masking_strategy == "both" ) && ( ch_fixed_mask || ch_moving_mask || ch_metric ) ) { | ||
| if ( ch_fixed_mask ) { | ||
| MASK_FIXED_IMAGE ( ch_fixed_image.join(ch_fixed_mask) ) | ||
| ch_fixed_image_ready = ch_fixed_image.join(MASK_FIXED_IMAGE.out.image, remainder: true) | ||
| .map({ meta, orig, masked -> [meta, masked?: orig] }) |
There was a problem hiding this comment.
Audit: Partially addressed. The subworkflow test config sets ext.masking_strategy = "apriori" for REGISTRATION_ANTS, and the SyNQuick test passes a fixed mask — so apriori with a fixed mask is exercised. However, no test covers 'both', 'internal', or the combination of fixed + moving masks. The ch_fixed_metric_ready bug (metric + no fixed mask + apriori) would not be caught by the current suite.
…ithub.com:arnaudbore/nf-neuro into move_mask_from_registration_module_to_subworkflow
| .branch{ | ||
| anat_to_dwi : it[3] | ||
| ants_syn: true | ||
| return it[0..2] + it[4..5] | ||
| } |
There was a problem hiding this comment.
Audit: Invalid. Nextflow's branch operator evaluates predicates sequentially — anat_to_dwi: it[3] captures metric-bearing items first; ants_syn: true is a catch-all default for items that didn't match the prior branch. Items do not route to both branches. An item with a truthy it[3] (metric) goes to anat_to_dwi only.
| UTILS_OPTIONS("${moduleDir}/meta.yml", options, true) | ||
| options = UTILS_OPTIONS.out.options.value | ||
|
|
||
| if ( ( options.masking_strategy == "apriori" || options.masking_strategy == "both" ) && ( ch_fixed_mask || ch_moving_mask || ch_metric ) ) { |
There was a problem hiding this comment.
Audit: Valid and still present at HEAD. This is the same root issue as the earlier comment on line 69. ch_fixed_metric_ready is undefined in the inner else (no fixed mask) when masking_strategy is apriori/both. Trigger: masking_strategy = "apriori" + metric supplied + no fixed mask → MissingPropertyException. Needs fixing.
There was a problem hiding this comment.
@arnaudbore I think this still needs addressing?
| moving_base=\$(basename "${moving_anat}") | ||
| ext=\${moving_base#*.} | ||
| moving_id=\${moving_base%.\${ext}} | ||
| moving_id=\$(basename $moving_anat .nii.gz) |
There was a problem hiding this comment.
Audit: Valid concern, but pre-existing — the basename $moving_anat .nii.gz hardcoding was in the original code before this PR (not a regression introduced here). The REGISTRATION_ANTS module handles this more robustly with ext=${moving_base#*.} / moving_id=${moving_base%.${ext}}. Worth aligning anattodwi to the same pattern for consistency, but it can be a follow-up.
PR #369 Review — Registration masking move to subworkflowOverall the architecture is sound: masking concerns move out of individual modules into the subworkflow, original unmasked images are re-warped post-registration via 🔴 Blocker:
|
| Location | Issue |
|---|---|
subworkflows/.../registration/meta.yml |
image_warped_masked and reference_warped_masked are emitted but not documented in outputs |
subworkflows/.../registration/meta.yml |
reference_warped still says "ONLY PROVIDED BY REGISTRATION_EASYREG" — now produced by all paths (ANTS, SynthMorph via WARP_IMAGE_TO_MOVING) |
modules/.../ants/meta.yml + anattodwi/meta.yml |
masking_strategy choices list 'none', 'apriori', 'both' — missing 'internal' which the code checks |
subworkflows/.../registration/meta.yml:99,112 |
Grammar: "will be use" → "will be used" |
🟡 Minor
-
applymask/main.nf:19— stray leading space in the else branch produces a double space in themrcalccommand:def data_type = task.ext.data_type ? "-datatype ${task.ext.data_type}" : " -datatype float32" // ^ extra leading space
-
No subworkflow test covers
masking_strategy: 'both'or'internal', nor any test with both fixed AND moving masks. The current SyNQuick test exercises'apriori'with a fixed mask only. Thech_fixed_metric_readybug above would not be caught by the current suite — a regression test forapriori+ metric (anattodwi) + no fixed mask would catch it. -
Test assertion key mismatch (pre-existing):
["ref_warped", "segmentation", "ref_segmentation"]in the subworkflow test doesn't match the actual emit namesreference_warped/reference_segmentation, so the empty-check assertion is a no-op.
✅ What's good
- Clean separation: modules stay dumb, subworkflow orchestrates masking.
- Re-warping original unmasked images via
antsApplyTransformsis the right call (addresses @gagnonanthony's original feedback about wanting the original image warped, not the masked one). - Backward compatible via
masking_strategy = "none"default. - Module-level tests added for mask inputs (anattodwi fixed/moving mask tests, ants both-moving-mask config).
applymaskfirst_suffix/data_typeoptions are clean generalizations.
Recommendation: Request changes — fix the ch_fixed_metric_ready blocker + add a regression test, address the *_warped_masked semantics, and update the meta.yml docs.
|
I'll have additional comments after a review of the impact of this on #282 |
Align cobralab_ants with the registration subworkflow masking rework (PR nf-neuro#369): gate fixed/moving mask forwarding behind masking_strategy (none/apriori/internal/both), add fixed_warped output (reference warped to moving space via antsApplyTransforms), and update tests accordingly.
|
@arnaudbore can you update the PR description with a full description of the overall intention and design of the implementation |
|
@arnaudbore what's the intention for what happens to the masked images on the output of the subworkflows |
Is that a rhetorical question for me to improve the description of the PR ? Otherwise I don't understand 😆 |
Its a question to clarify what the design intention is here. What I'm expecting:
I'm trying understand state before and after the change, as this will impact both the callers of the subworkflow, and the consumers afterwards. |
|
Great ! After this change, the registration subworkflow still accepts the same required and optional inputs. The only difference is that, when a mask is provided, it is passed internally to the new selected registration method: masking_strategy (apriori, internal or both) to improve the registration and it will emit a respecting reference_warped_masked and/or image_warped_masked channels along with the already previously existing image_warped and reference_warped ones. I hope it's clear |
gdevenyi
left a comment
There was a problem hiding this comment.
Thanks for pushing this forward — the none / apriori / internal / both taxonomy is the right shape and it answers the earlier thread with @AlexVCaron and @gagnonanthony well. Keeping a-priori masking and in-registration masking as independent, combinable options is the correct call.
I ran this rather than only reading it (nf-test 0.9.3, apptainer, scilus/scilus:2.2.2, plus a local ANTs 2.6.5 build for flag semantics). Unfortunately the feature does not currently work, and green CI is actively misleading here — no test sets masking_strategy on the subworkflow, so none of the new code paths are exercised.
Four blocking issues, each reproduced:
1. apriori / both abort the run
All three new aliases are IMAGE_APPLYMASK and none set ext.first_suffix, so all three write ${prefix}_masked.nii.gz. As soon as two masked outputs reach the same process, Nextflow aborts:
ERROR ~ Error executing process > 'REGISTRATION:REGISTRATION_ANTS (1)'
Caused by:
Process `REGISTRATION:REGISTRATION_ANTS` input file name collision --
There are multiple input files for each of the following file names: test_masked.nii.gz
The anat-to-DWI path hits the same thing one process over (MASK_FIXED_IMAGE + MASK_FIXED_METRIC). This PR already adds ext.first_suffix to image/applymask — that is the fix, it just is not used here. The subworkflow ships no modules.config (unlike tractoflow), so there is nowhere for it to live yet.
2. internal / both is a silent no-op in registration/anattodwi
The -x flag is built in antsRegistrationSyN.sh syntax but handed to raw antsRegistration, which wants --masks [fixed,moving]. ANTs treats the unreadable name as no mask, warns non-fatally and carries on. Your own snapshot proves the effect is nil — registration - anattodwi - fixed mask is byte-identical to the no-mask test on every output md5. Details inline.
3. Both new subworkflow tests exercise the wrong module
take: order is ..., ch_metric(2), ch_fixed_mask(3), ch_moving_mask(4), but the tests pass masks at input[2]/input[3]. The fixed mask binds to ch_metric, which routes to REGISTRATION_ANATTODWI — so REGISTRATION_ANTS never runs and the withName: "REGISTRATION_ANTS" configs (including the masking_strategy they exist to set) apply to nothing. Confirmed two ways from the committed snapshots: no mqc despite ext.run_qc = true, and the versions md5s are anattodwi + antsapplytransforms with the ants one absent.
4. The new re-warp ignores masking_strategy
WARP_IMAGE_TO_FIXED / WARP_IMAGE_TO_MOVING are gated only on a mask being present, so masking_strategy = "none" plus a mask still spawns an extra antsApplyTransforms and replaces the module output. Visible in your own snapshot: registration - ANTs - SyNQuick passes input[8] = [:] and still emits reference_warped = test_T1w_warped.nii.gz instead of test_warped_reference.nii.gz.
One design question worth settling before this lands
REGISTRATION_ANTS used to mask whenever masks were supplied. It now silently ignores them unless ext.masking_strategy is also set. Every existing pipeline passing masks quietly loses in-registration masking, with no warning. This repo is already an instance — output_template_space needed ext.masking_strategy = "internal" added to its test config to preserve behaviour, but real consumers get no such default and no way to set it (see the output_template_space comment below).
Would you consider leaving the modules as they were (masks given ⇒ masks used) and letting the subworkflow decide whether to forward masks at all? It already owns the strategy, so it can simply pass [] for none/apriori. That keeps one knob in one place instead of duplicating masking_strategy into two modules where two of its four values are no-ops.
Suggested order
- Add
subworkflows/nf-neuro/registration/modules.configwith per-aliasext.first_suffix— unblocksapriori/both. - Bracket the
--masksargument inanattodwi. - Fix
input[2..4]andinput[8]in the two new subworkflow tests; re-record and confirmREGISTRATION_ANTSactually appears inversions. - Guard the two
WARP_IMAGE_TO_*blocks onapriori/both. - Add a test that sets
input[8] = ["masking_strategy": "apriori"]with both masks — the only thing that would have caught 1 and 4.
Happy to re-review quickly once these are in. The direction is good; it is the wiring and the test coverage that need another pass.
| def run_qc = task.ext.run_qc as Boolean || false | ||
| def args = task.ext.args ?: '' | ||
|
|
||
| if (( task.ext.masking_strategy == "both" || task.ext.masking_strategy == "internal" ) && (fixed_mask || moving_mask)) args += " -x \"${fixed_mask ?: 'NULL'},${moving_mask ?: 'NULL'}\"" |
There was a problem hiding this comment.
Blocking — this masks nothing, silently.
This is antsRegistrationSyN.sh syntax, but the process below calls raw antsRegistration, whose option is -x, --masks [fixedImageMask,movingImageMask]. ANTs parses T1w_mask.nii.gz,NULL as a single filename, cannot read it, and treats an unreadable name as no mask rather than an error. Verified against ANTs 2.6.5:
| argv reaching ANTs | verbose output |
|---|---|
T1w_mask.nii.gz,NULL (this line) |
file ... does not exist . → No fixed mask |
[mask.nii.gz,NULL] |
Fixed mask = mask.nii.gz / No moving mask |
[NULL,mask.nii.gz] |
No fixed mask / Moving mask = mask.nii.gz |
[mask.nii.gz,mask.nii.gz] |
both applied |
The warning is non-fatal and the exit code stays 0, so nothing fails — which is why CI is green. The committed snapshot confirms it: registration - anattodwi - fixed mask and registration - anattodwi (no mask) are byte-identical on every md5, e.g. anat_warped is 9f2eda1485017da83b5fcd6871cdcb74 in both. Running the test locally:
$ grep -o '\-x .*' .command.sh
-x "T1w_mask.nii.gz,NULL"
$ grep -i 'does not exist' .command.err
file T1w_mask.nii.gz,NULL does not exist .
The fix is just the brackets — this is precisely what antsRegistrationSyNQuick.sh:339 does with its own -x value, which is why the same string works in registration/ants and not here:
if (( task.ext.masking_strategy == "both" || task.ext.masking_strategy == "internal" ) && (fixed_mask || moving_mask)) args += " -x \"[${fixed_mask ?: 'NULL'},${moving_mask ?: 'NULL'}]\""Please keep the surrounding quotes and move the brackets inside them. Nextflow interpolates args as bash source, so bash strips the quotes and ANTs receives [fixed,moving]. Unquoted, the bracket is a valid bash pathname glob and can silently collapse:
$ touch T
$ ... -x "[T1w_mask.nii.gz,NULL]" → argv=<[T1w_mask.nii.gz,NULL]> # safe
$ ... -x [T1w_mask.nii.gz,NULL] → argv=<T> # glob-expanded
Two follow-ups:
- A single
--masksis broadcast to all stages (itkantsRegistrationHelper.hxx:867,if (m_FixedImageMasks.size() == 1) fixedMaskIndex = 0;), so appending once is correct — no per-stage repetition needed. - Worth making the test able to fail: assert the two snapshots differ, or grep
.command.errfordoes not exist. As written, the test that exists to prove masking works instead proves it never happens.
| $args | ||
|
|
||
| moving_base=\$(basename "${moving_anat}") | ||
| moving_base=\$(basename $moving_anat .nii.gz) |
There was a problem hiding this comment.
Minor: basename $moving_anat .nii.gz already strips the extension, so the two following lines are now no-ops — ${moving_base#*.} on a string with no dot returns it unchanged, and ${moving_base%.${ext}} then matches nothing. The result is right by accident. The quoting around $moving_anat was also dropped here.
The previous three-line form was correct; suggest reverting rather than keeping dead code (same pattern was copied into stub: at L145-147).
|
|
||
| input: | ||
| tuple val(meta), path(fixed_reference), path(moving_anat), path(metric) | ||
| tuple val(meta), path(fixed_reference), path(moving_anat), path(metric), path(fixed_mask), path(moving_mask) |
There was a problem hiding this comment.
Worth calling out in the PR description / release notes: this changes the input cardinality from 4 to 6, so every external caller of REGISTRATION_ANATTODWI must add [], []. It fails loudly rather than silently, so it is much less dangerous than the masking_strategy default change — but it is still a breaking API change for anyone who has installed this module.
| if ( task.ext.histogram_matching ) args += " -j $task.ext.histogram_matching" | ||
| if ( task.ext.repro_mode ) args += " -y $task.ext.repro_mode" | ||
| if ( task.ext.collapse_output ) args += " -z $task.ext.collapse_output" | ||
| if ( (task.ext.masking_strategy == "both" || task.ext.masking_strategy == "internal") && (fixed_mask || moving_mask) ) args += " -x \"${fixed_mask ?: 'NULL'},${moving_mask ?: 'NULL'}\"" |
There was a problem hiding this comment.
This is the silent backward-compatibility break I raised in the summary.
Before this PR the module used masks whenever they were supplied. Now masks are accepted and ignored unless the caller also sets ext.masking_strategy, with no warning. Existing pipelines that pass masks will keep running and quietly produce different (unmasked) registrations.
This repo is already affected: output_template_space passes ch_brain_mask as ch_fixed_mask, and its test config had to gain ext.masking_strategy = "internal" in this PR to preserve behaviour. Real consumers of that subworkflow get no such default and no way to set it.
The mechanics here are correct, incidentally — antsRegistrationSyNQuick.sh:339 wraps a comma-containing -x value in brackets itself, so -x "fixed,NULL" works fine on this path (it emits a harmless file NULL does not exist warning). The problem is only the new opt-in gate.
Suggestion in the summary: drop masking_strategy from the modules and let the subworkflow pass [] when it does not want in-registration masking. Two of the four enum values (none, apriori) are no-ops at this level anyway.
| ch_fixed_metric_ready = ch_metric | ||
| if ( ( options.masking_strategy == "apriori" || options.masking_strategy == "both" ) && ( ch_fixed_mask || ch_moving_mask || ch_metric ) ) { | ||
| if ( ch_fixed_mask ) { | ||
| MASK_FIXED_IMAGE ( ch_fixed_image.join(ch_fixed_mask) ) |
There was a problem hiding this comment.
Blocking — apriori and both abort the run.
MASK_FIXED_IMAGE, MASK_FIXED_METRIC and MASK_MOVING_IMAGE are all IMAGE_APPLYMASK and none sets ext.first_suffix, so all three emit ${prefix}_masked.nii.gz. Whenever two of them feed the same downstream process, Nextflow aborts before the process runs.
Reproduced with a subworkflow test using input[8] = ["masking_strategy": "apriori"] plus a fixed and a moving mask, wired to the real take: order:
ERROR ~ Error executing process > 'REGISTRATION:REGISTRATION_ANTS (1)'
Caused by:
Process `REGISTRATION:REGISTRATION_ANTS` input file name collision --
There are multiple input files for each of the following file names: test_masked.nii.gz
And on the anat-to-DWI path, where the masked fixed image and masked metric collide:
Process REGISTRATION:REGISTRATION_ANATTODWI > collision check staging file names:
[test_masked.nii.gz:2, moving.nii.gz:1, moving_mask.nii.gz:1]
So both strategies fail for the ordinary case of supplying two masks. This PR already adds ext.first_suffix to image/applymask — it just needs using. The subworkflow has no modules.config yet (tractoflow has one); adding it would fix this:
process {
withName: ".*:MASK_FIXED_IMAGE" { ext.first_suffix = "fixed" }
withName: ".*:MASK_FIXED_METRIC" { ext.first_suffix = "metric" }
withName: ".*:MASK_MOVING_IMAGE" { ext.first_suffix = "moving" }
}| file("\${test_data_directory}/T1w.nii.gz").copyTo( | ||
| "\${test_data_directory}/moving.nii.gz") | ||
| ]} | ||
| input[2] = ch_split_test_data.t1w.map{ |
There was a problem hiding this comment.
Blocking — the masks are going into the wrong slots.
The subworkflow's take: order is:
ch_fixed_image(0), ch_moving_image(1), ch_metric(2), ch_fixed_mask(3), ch_moving_mask(4), ...
so fixed_mask here binds to ch_metric and moving_mask at L193 binds to ch_fixed_mask. Because ch_metric is now non-empty, the branch at main.nf:210 routes to REGISTRATION_ANATTODWI, which means:
REGISTRATION_ANTSnever runs, sonextflow_ANTS_internal.config/nextflow_ANTS_both.configapply to nothing — including themasking_strategythey were added to set;- a binary brain mask is routed into the CC metric slot;
- the tests named "ANTs - SyNQuick" test neither ANTs nor SyNQuick.
Two independent confirmations from the committed snapshot. Both configs set ext.run_qc = true, yet neither snapshot has an mqc channel. And the versions md5s are 03b958f1… + 3764dd2f… (anattodwi + antsapplytransforms); the ants md5 a338911f…, present in the real SyNQuick test, is absent.
Should be input[2] = channel.empty(), input[3] = fixed_mask, input[4] = moving_mask. Same fix in the - both test at L256/L262.
| input[5] = channel.empty() | ||
| input[6] = channel.empty() | ||
| input[7] = channel.empty() | ||
| input[8] = [:] |
There was a problem hiding this comment.
This is the other half of why the new paths are untested: input[8] = [:] leaves options.masking_strategy at its "none" default, so the subworkflow's a-priori masking, the masked output channels and the re-warp are all skipped. The ext.masking_strategy in the test config only reaches the module.
For these two tests to mean what their names say, this needs to be ["masking_strategy": "internal"] and ["masking_strategy": "both"] respectively (L272). Worth adding an apriori case too — that is the one that surfaces the filename collision.
| cpus = 1 | ||
|
|
||
| withName: "REGISTRATION_ANTS" { | ||
| ext.masking_strategy = "apriori" |
There was a problem hiding this comment.
Copy/paste: this sets apriori, not internal, so it does not match the file name or the test name. Note that once the input wiring above is fixed, internal and both will still be indistinguishable at this level unless the subworkflow option is also set — at module level apriori and none are both no-ops, and both and internal are identical.
| [ | ||
| "run_easyreg": options.run_easyreg, | ||
| "run_synthmorph": options.run_synthmorph, | ||
| "masking_strategy": options.masking_strategy |
There was a problem hiding this comment.
masking_strategy is not declared in this subworkflow's meta.yml options, and UTILS_OPTIONS runs in strict mode (it logs Unknown option ... will be ignored), so options.masking_strategy is always null here. Passing it therefore overrides REGISTRATION's "none" default with null, and callers of OUTPUT_TEMPLATE_SPACE have no way to set it.
params.masking_strategy = "internal" added to tests/nextflow.config has no effect either — options come from the map argument, not from params.
Either declare the entry in output_template_space/meta.yml so it can actually be configured, or drop this line.
| -T ${prefix}_backward\${i}_\$model.\${extension[\$model]} \ | ||
| -o warped.nii.gz -j ${nthreads} $extent $use_gpu | ||
| -o warped.nii.gz \ | ||
| -O fixed_warped.nii.gz \ |
There was a problem hiding this comment.
Minor: moving is reassigned to warped.nii.gz between deform stages (L118), but fixed_warped.nii.gz is overwritten on every iteration and never chained. That is fine for the default ["affine", "deform"], where the affine stage only sets an initializer — but with two deform stages the emitted *_warped_reference.nii.gz is the fixed image mapped into the intermediate space rather than the original moving space.
Not blocking given the defaults; worth a comment or a guard so it does not surprise someone who configures ext.models.
Follow-up: integration with #282 (
|
| module | how masks are expressed |
|---|---|
registration/ants |
-x "fixed,moving" — works, because antsRegistrationSyNQuick.sh:339 brackets it internally |
registration/anattodwi |
raw antsRegistration, needs -x "[fixed,moving]" — currently a silent no-op |
registration/cobralab_ants (#282) |
--fixed-mask X --moving-mask Y |
easyreg / synthmorph |
no mask support at all |
The mechanism is legitimately per-module. The policy — none/apriori/internal/both — is byte-identical in all of them and is already duplicated into #282, where (as here) two of the four values are no-ops at module level. Worth noting that #282's implementation is the cleanest of the four precisely because separate flags never force it to encode anything into an argument string.
If the enum lives only in the subworkflow and the modules keep plain masks given ⇒ masks used, then #282 needs zero policy code and its --fixed-mask/--moving-mask block stands exactly as written.
There is a real cost to deferring this. #282 already documents a feature in terms of the enum — mask_extract: "Requires masks to be forwarded to the tool (masking_strategy = 'internal' or 'both')". Changing the contract after both merge means touching three modules plus their docs, instead of one subworkflow now.
2. run_easyreg / run_synthmorph will not survive a fourth backend
The meta.yml descriptions are already mutually recursive — "If false, SynthMorph will be used if run_synthmorph is true, and ANTs SyN will be used if run_synthmorph is false". Wiring cobralab in adds a third boolean: 8 states, 5 of them meaningless, and precedence defined only by if/else ordering at L71 / L106 / L193.
This PR is already reshaping the options schema to add masking_strategy, so it is the cheap moment to collapse them:
method:
type: string
choices: [ants, anattodwi, easyreg, synthmorph]
default: antsThen #282 adds one enum value and one branch, instead of a fourth boolean whose interaction with the other three is undefined.
3. The incompatibility check at L67 is a hardcoded backend list — and would be wrong for cobralab
if ( ( options.masking_strategy == "both" || options.masking_strategy == "internal" ) && ( options.run_easyreg || options.run_synthmorph ) ) {This enumerates the backends that cannot mask internally. cobralab_ants can, so whoever wires it in has to remember not to add it here — and getting that wrong produces a spurious hard error that no existing test would catch. Keyed off a method enum this becomes a capability lookup rather than a list somebody has to maintain by hand.
Smaller integration notes
- Suffix contract. This PR's description says registration modules need to define a suffix, and
registration/ants/meta.ymlnow declaressuffixas "Obligatory suffix to add to the output file name" (defaultwarped). Implementation of antsRegistration_affine_SyN.sh for registration #282 declares nosuffixarg and hardcodes${prefix}_${moving_id}_warped.nii.gz. Worth writing the contract into the subworkflow'smeta.ymlso new backends can conform to it rather than discover it. choices:—antsandanattodwideclare the four-value enum here; Implementation of antsRegistration_affine_SyN.sh for registration #282's copy ofmasking_strategyis a baretype: stringwith nochoices. Align whichever way this lands.- The
_masked.nii.gzcollision fix covers Implementation of antsRegistration_affine_SyN.sh for registration #282 for free. Addingmodules.configwith per-aliasext.first_suffixin this PR means cobralab inherits a workingapriori/boththe moment it is wired in. Leaving it to the consumer means fixing it once per consumer. - mqc filename. Implementation of antsRegistration_affine_SyN.sh for registration #282 emits
*_registration_ants_mqc.gif— the same nameregistration/antsuses. Not this PR's problem, but if the subworkflow mixesmqcfrom whichever backend ran, the two ought to be distinguishable in the MultiQC report. - Container skew:
scilus/scilus:2.2.2here vs2.3.0in Implementation of antsRegistration_affine_SyN.sh for registration #282 (which needs 2.3.0 forantsRegistration_affine_SyN.sh).
The good news
#282 is otherwise a drop-in for the ANTs branch. It emits every channel the subworkflow consumes — image_warped, fixed_warped, forward_affine, forward_warp, backward_affine, backward_warp, {forward,backward}_{image,tractogram}_transform, mqc, versions — with the same globs, and its transforms are ordinary ANTs .mat / warp fields, so the WARP_IMAGE_TO_* re-warp works with it once gated.
The fixed_warped / *_warped_reference.nii.gz contract this PR introduces is already implemented there, and arguably more robustly: #282 computes it with an explicit antsApplyTransforms using the backward transforms, rather than relying on antsRegistrationSyN*.sh always emitting outputInverseWarped.nii.gz. If you want a belt-and-braces version of registration/ants, that is the pattern.
Align cobralab_ants with the registration subworkflow masking rework (PR nf-neuro#369): gate fixed/moving mask forwarding behind masking_strategy (none/apriori/internal/both), add fixed_warped output (reference warped to moving space via antsApplyTransforms), and update tests accordingly.
Type of improvement
New registration implementation and update registration modules.
Registration modules needs to define a suffix.
Subworkflow registration add a new parameters: masking_strategy
masking_strategy choices are: none, apriori, internal or both
If submitting a new module or fixing a bug, please use the appropriate template.
Describe your improvement
Write a clear and concise description of what the improvement is.
Describe how to test your improvement
Provide a full step-by-step guide to test your improvement.
Checklist before requesting a review