Skip to content

ENH: Add option to coregister BOLDs to "session" space#517

Open
mgxd wants to merge 26 commits into
masterfrom
docker/coreg-bolds
Open

ENH: Add option to coregister BOLDs to "session" space#517
mgxd wants to merge 26 commits into
masterfrom
docker/coreg-bolds

Conversation

@mgxd
Copy link
Copy Markdown
Collaborator

@mgxd mgxd commented Jan 28, 2026

This PR adds in --coreg-bolds, which generates a shared BOLD "session" reference space.

If enabled, a template is created from each BOLD using mri_robust_template.

This change required a significant reorganization of the workflow, essentially stripping out much of the logic within init_bold_wf into the base workflow.

One important caveat is this is not intended to be used if no SDC is to be applied and BOLD runs do not have a consistent phase encoding direction.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 28, 2026

Codecov Report

❌ Patch coverage is 91.11111% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.53%. Comparing base (23c6af6) to head (5d8bffa).
⚠️ Report is 42 commits behind head on master.

Files with missing lines Patch % Lines
nibabies/workflows/base.py 87.50% 4 Missing and 3 partials ⚠️
nibabies/workflows/bold/base.py 85.71% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #517      +/-   ##
==========================================
+ Coverage   57.21%   57.53%   +0.31%     
==========================================
  Files          72       73       +1     
  Lines        6977     7010      +33     
  Branches      860      860              
==========================================
+ Hits         3992     4033      +41     
+ Misses       2763     2756       -7     
+ Partials      222      221       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mgxd mgxd marked this pull request as ready for review January 30, 2026 16:20
@mgxd mgxd changed the title ENH: Add option to coregister BOLD ENH: Add option to coregister BOLDs to "session" space Jan 30, 2026
Comment thread nibabies/cli/parser.py
'registrations (native -> MNIInfant -> template) and concatenate into a single xfm',
)
g_baby.add_argument(
'--coreg-bolds',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't love this name. I'm kind of thinking something like --coreg-ref run/session, that allows us to potentially support other uses cases in the future.

Not sure that's a great alternative, but just to share where my brain's at and maybe that will trigger something for you. I'm also okay with keeping this and maybe/maybe not deprecating it in the future.

fields=['fmap', 'fmap_ref', 'fmap_coeff', 'fmap_mask', 'sdc_method'],
key=fieldmap_id,
),
name=f'fmap_select{i}',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For debugging purposes, could we use the same entity concatenation that we do in bold_<entities>_wf instead of i?

('coreg_boldref', 'inputnode.boldref'),
('bold_mask', 'inputnode.bold_mask'),
('motion_xfm', 'inputnode.motion_xfm'),
('boldref2fmap_xfm', 'inputnode.boldref2fmap_xfm'),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that boldref is no longer a stand-in for "orig modulo motion", I wonder if we should rename boldref2fmap_xfm as orig2fmap_xfm globally. Right now, you could imagine that it's being registered to the intermediate boldref, which would require different handling.

('outputnode.boldref2anat_xfm', 'inputnode.boldref2anat_xfm'),
('outputnode.motion_xfm', 'inputnode.motion_xfm'),
('outputnode.boldref2fmap_xfm', 'inputnode.boldref2fmap_xfm'),
(inputnode, ds_bold_anat_wf, [
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you not need to pass orig2boldref_xfm here as well?

Comment on lines +659 to +663
('bold_mask', 'inputnode.bold_mask'),
('coreg_boldref', 'inputnode.hmc_boldref'),
('motion_xfm', 'inputnode.motion_xfm'),
('boldref2anat_xfm', 'inputnode.boldref2anat_xfm'),
('dummy_scans', 'inputnode.skip_vols'),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we're missing the orig2boldref in the confounds workflow as well.

Though now I start to wonder if what we want are buffers where we populate the transform chains, like:

orig2ses_xfm = Merge(2)  # HMC + orig2boldref
orig2anat_xfm = Merge(2)  # orig2ses + boldref2anat
orig2std_xfm = Merge(2)  # orig2anat + anat2std

Then workflows identify what specific chains they want, rather than the components that each has to sequence properly.

'boldref',
'bold_mask',
'motion_xfm',
'orig2boldref_xfm',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

native is supposed to be in orig space, not session. I think this workflow should be reverted, modulo the fieldmap changes.

Comment on lines +328 to +333
to_fmap_xfm = pe.Node(
niu.Merge(2),
name='to_fmap_xfm',
run_without_submitting=True,
mem_gb=config.DEFAULT_MEMORY_MIN_GB,
)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per out-of-band discussion, the fieldmap is registered directly to the run-space boldref, so orig2boldref_xfm should not be used here.

else:
from niworkflows.data import load as nwf_load

boldref_buffer.inputs.orig2boldref_xfm = nwf_load('itkIdentityTransform.txt')
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this even necessary? Generally leaving a transform empty has been a safe way of making generic workflows, e.g., omitting the boldref2anat_xfm resamples to boldref instead of anat.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants