Implementation of antsRegistration_affine_SyN.sh for registration#282
Implementation of antsRegistration_affine_SyN.sh for registration#282gdevenyi wants to merge 6 commits into
Conversation
390c9c1 to
a90da89
Compare
|
Minimal work is required so the module fits with the Once done, most linting and testing should work out the box. But nf-test expects some naming/formatting, that sadly isn't targetted when nf-core conventions are not abided to. |
2878c25 to
ae312c2
Compare
|
@AlexVCaron I can't sort out how to run the test properly inside the devcontainer, what's the appropriate command? |
ae312c2 to
b91bb4a
Compare
|
I run the test suite for any component from the root of the repo via : In this case, the path is For the lint, it is done via the Tell me of any problems, I can pull your PR and help debug ! |
|
Right. OK. It seems like the devcontainer doesn't have the antsRegistration_affine_SyN.sh that I got merged into the main repo... |
|
I'll get into that this w-e. It should've been included in the build, I'll investigate |
|
As stated on scilus/containers-scilus#42, |
0932c5d to
a7e2ecb
Compare
4db49ca to
b09a2af
Compare
|
@gdevenyi container is updated, we still have some issues with our servers I'll let you know asap when ready. |
No problem my local testbed works. |
b09a2af to
e9318a2
Compare
e9318a2 to
191152d
Compare
Split single mask input into separate fixed_mask and moving_mask inputs, matching the pattern from PR nf-neuro#367 for the ants registration module. antsRegistration_affine_SyN.sh supports --fixed-mask and --moving-mask as independent flags.
|
Depends on scilus/containers-scilus#45, also need to update the container reference once its released |
Merged and scilus/scilus:dev is updated |
|
This all works, I guess the only thing now is this should be pinned to an official scilus container version instead of dev |
|
I'm not sure there will be a release with a new version soon, right @arnaudbore ? In the meantime, you could pin it to the image SHA. |
Sure. |
…hread support - Forward --reproducibility and --random-seed CLI flags to antsRegistration_affine_SyN.sh (CoBrALab PR nf-neuro#7) - Export ANTS_RANDOM_SEED env var (default 1234) for ancillary ANTs tools - Replace hardcoded thread counts with task.ext.single_thread-aware expressions for ITK_GLOBAL_DEFAULT_NUMBER_OF_THREADS and OMP_NUM_THREADS - Remove OPENBLAS_NUM_THREADS (set globally in test config) - Add reproducibility, ants_rng_seed, single_thread args to meta.yml Mirrors the pattern already applied to registration/ants (nf-neuro PR nf-neuro#342).
3ec0077 to
cb0a599
Compare
|
The masking implementation here depends on #369, if that's changed this should be as well |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a new registration/cobralab_ants Nextflow module to nf-neuro, including module metadata and an nf-test suite covering default, “quick”, “no warp”, and stub modes.
Changes:
- Introduces the
REGISTRATION_COBRALABANTSprocess implementation and module metadata. - Adds
nf-testcoverage and snapshots for multiple registration configurations. - Adds test tagging and per-test Nextflow config overrides.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/nf-neuro/registration/cobralab_ants/main.nf | Adds the REGISTRATION_COBRALABANTS process implementation, including optional QC generation and stub behavior. |
| modules/nf-neuro/registration/cobralab_ants/meta.yml | Documents module inputs/outputs/tools and parameters used by the process. |
| modules/nf-neuro/registration/cobralab_ants/environment.yml | Adds module conda environment placeholder for linting/metadata consistency. |
| modules/nf-neuro/registration/cobralab_ants/tests/main.nf.test | Adds nf-test cases for default, quick, no-warp, and stub runs. |
| modules/nf-neuro/registration/cobralab_ants/tests/main.nf.test.snap | Adds snapshots asserting expected emitted outputs/hashes for each test. |
| modules/nf-neuro/registration/cobralab_ants/tests/nextflow.config | Test config enabling reproducibility + single-thread mode. |
| modules/nf-neuro/registration/cobralab_ants/tests/nextflow_quick.config | Test config enabling fast mode + QC + single-thread mode. |
| modules/nf-neuro/registration/cobralab_ants/tests/nextflow_no_warp.config | Test config enabling fast + linear-only (no nonlinear warp) + single-thread mode. |
| modules/nf-neuro/registration/cobralab_ants/tests/tags.yml | Adds tags mapping to allow selective test execution by module path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for image in fixed_image warped; do | ||
| mrconvert *\${image}.nii.gz *\${image}_viz.nii.gz -stride -1,2,3 | ||
| scil_viz_volume_screenshot *\${image}_viz.nii.gz \${image}_coronal.png \ | ||
| --slices \$coronal_dim --axis coronal \$viz_params | ||
| scil_viz_volume_screenshot *\${image}_viz.nii.gz \${image}_sagittal.png \ | ||
| --slices \$sagittal_dim --axis sagittal \$viz_params | ||
| scil_viz_volume_screenshot *\${image}_viz.nii.gz \${image}_axial.png \ | ||
| --slices \$axial_dim --axis axial \$viz_params |
There was a problem hiding this comment.
Addressed in 587282e — the QC viz calls now use a concrete output filename instead of a glob on both input and output positions.
| input: | ||
| tuple val(meta), path(fixed_image), path(moving_image), path(fixed_mask), path(moving_mask) //** optional, input = [] **// |
There was a problem hiding this comment.
Invalid for this codebase — the //** optional, input = [] **// convention is how nf-neuro modules pass optional tuple file inputs (the test harness emits [], []). The declaration is identical to the canonical registration/ants module, which CI validates; meta.yml's mandatory: false remains the source of truth.
| moving_id=\$(basename $moving_image .nii.gz) | ||
| moving_id=\${moving_id#${meta.id}_*} |
There was a problem hiding this comment.
Addressed in 587282e — the .nii.gz assumption was resolved by adopting the extension-agnostic basename parsing from the registration/ants reference module, so both .nii and .nii.gz inputs are handled.
|
|
||
| ### ** QC ** ### | ||
| if $run_qc; then | ||
| mv $fixed_image fixed_image.nii.gz |
There was a problem hiding this comment.
Addressed in 587282e (see reply above) — extension-agnostic basename parsing now handles both .nii and .nii.gz.
| fixed_id=\$(basename $fixed_image .nii.gz) | ||
| fixed_id=\${fixed_id#${meta.id}_*} |
There was a problem hiding this comment.
Addressed in 587282e (see reply above) — extension-agnostic basename parsing now handles both .nii and .nii.gz.
| process { | ||
| withName: "REGISTRATION_COBRALABANTS" { | ||
| publishDir = { "${params.outdir}/${task.process.tokenize(':')[-1].tokenize('_')[0].toLowerCase()}" } | ||
| ext.reproducibility = true | ||
| ext.single_thread = true |
There was a problem hiding this comment.
Invalid — this publishDir expression is the established convention across every registration module test config in the repo (ants, anattodwi, convert, easyreg, synthmorph). Each config is scoped to a single process via withName, so there is no collision in practice.
| type: list | ||
| description: | | ||
| Tuple, Transformation files to warp images in fixed space, in the correct order | ||
| for REGISTRATION_TRANSFORMTRACTOGRAM : [ meta, [ forward_warp, forward_affine ] ]. |
There was a problem hiding this comment.
Invalid here — this wording is word-for-word identical to the canonical registration/ants module's forward_image_transform/backward_image_transform descriptions. A cobralab-only edit would diverge from the reference; this should be a separate repo-wide fix across ants/anattodwi/cobralab_ants.
| description: | | ||
| Tuple, transformation files to warp images in moving space, in the correct order | ||
| for REGISTRATION_TRANSFORMTRACTOGRAM : [ meta, [ backward_affine, backward_warp ] ]. |
There was a problem hiding this comment.
Invalid here — wording is identical to the canonical registration/ants module; should be a separate repo-wide fix rather than a cobralab-only change.
|
@gdevenyi scilus/scilus:2.3.0 is ready 👍 |
|
Cool. Still pending confirmation of the masking implementation in #369 |
cb0a599 to
235e030
Compare
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.
| moving_id=\$(basename $moving_image .nii.gz) | ||
| moving_id=\${moving_id#${meta.id}_*} |
There was a problem hiding this comment.
Addressed in 587282e — ID parsing is now extension-agnostic (handles both .nii and .nii.gz) and strips by ${prefix} instead of ${meta.id}, matching the registration/ants reference module.
| fixed_id=\$(basename $fixed_image .nii.gz) | ||
| fixed_id=\${fixed_id#${meta.id}_*} |
There was a problem hiding this comment.
Addressed in 587282e — fixed_id is now parsed extension-agnostically and stripped by ${prefix}, consistent with the moving_id parsing and the registration/ants reference module.
| mrconvert *\${image}.nii.gz *\${image}_viz.nii.gz -stride -1,2,3 | ||
| scil_viz_volume_screenshot *\${image}_viz.nii.gz \${image}_coronal.png \ | ||
| --slices \$coronal_dim --axis coronal \$viz_params | ||
| scil_viz_volume_screenshot *\${image}_viz.nii.gz \${image}_sagittal.png \ | ||
| --slices \$sagittal_dim --axis sagittal \$viz_params | ||
| scil_viz_volume_screenshot *\${image}_viz.nii.gz \${image}_axial.png \ | ||
| --slices \$axial_dim --axis axial \$viz_params |
There was a problem hiding this comment.
Addressed in 587282e — the QC mrconvert/scil_viz_volume_screenshot calls now use a concrete output filename (${image}_viz.nii.gz) instead of a glob, matching the registration/ants reference module.
…ames
Address Copilot review: parse moving/fixed IDs extension-agnostically
(.nii and .nii.gz) and strip by ${prefix} instead of ${meta.id}; use a
concrete viz output filename instead of a glob. Matches the registration/ants
reference module.
Describe your changes
Implement the CoBrALab variant of antsRegistrationSyN.sh with optimized pyramids.
List test packages used by your module
Checklist before requesting a review
./modules/nf-neuro/<category>/<tool>/main.nf./modules/nf-neuro/<category>/<tool>/meta.yml./modules/nf-neuro/<category>/<tool>/environment.yml./modules/nf-neuro/<category>/<tool>/tests/main.nf.testmain.nf.test.snapsnapshotsprettierandeditorconfig-checkerto fix common syntax issuesnf-core modules lintand fix all errors