New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RF] Split BOLD-T1w registration into calculation/application workflows #1278
Conversation
1954e22
to
2a7f896
Compare
2a7f896
to
b245016
Compare
4dee39b
to
5b01202
Compare
5b01202
to
5d9f624
Compare
cbc9f5d
to
d13d1a1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Further comments in person.
fmriprep/workflows/bold/base.py
Outdated
@@ -211,7 +211,8 @@ def init_func_preproc_wf(bold_file, ignore, freesurfer, | |||
* :py:func:`~fmriprep.workflows.bold.stc.init_bold_stc_wf` | |||
* :py:func:`~fmriprep.workflows.bold.hmc.init_bold_hmc_wf` | |||
* :py:func:`~fmriprep.workflows.bold.t2s.init_bold_t2s_wf` | |||
* :py:func:`~fmriprep.workflows.bold.registration.init_bold_reg_wf` | |||
* :py:func:`~fmriprep.workflows.bold.registration.init_bold_apply_reg_wf` | |||
* :py:func:`~fmriprep.workflows.bold.registration.init_bold_calc_reg_wf` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, let's call the apply registration workflow and init_bold_t1_trans_wf
. We could then go back to init_bold_reg_wf
or init_bold_calc_reg_wf
; either one.
88c83d9
to
4e7b8b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aesthetic suggestions. This looks very good. Thanks!
fmriprep/workflows/bold/base.py
Outdated
('t1_mask', 'inputnode.t1_mask'), | ||
('t1_aseg', 'inputnode.t1_aseg'), | ||
('t1_aparc', 'inputnode.t1_aparc')]), | ||
(inputnode, bold_t1_trans_wf, [('bold_file', 'inputnode.name_source')]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be merged with the previous (inputnode, bold_t1_trans_wf
connection.
fmriprep/workflows/bold/base.py
Outdated
(bold_t1_trans_wf, outputnode, [('outputnode.bold_aseg_t1', 'bold_aseg_t1'), | ||
('outputnode.bold_aparc_t1', 'bold_aparc_t1')]), | ||
(bold_reg_wf, bold_t1_trans_wf, [ | ||
('outputnode.itk_bold_to_t1', 'inputnode.itk_bold_to_t1')]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer this connection to go before the bold_t1_trans_wf -> outputnode
one.
fmriprep/workflows/bold/base.py
Outdated
('outputnode.bold_aparc_t1', 'bold_aparc_t1')]), | ||
(bold_reg_wf, bold_t1_trans_wf, [ | ||
('outputnode.itk_bold_to_t1', 'inputnode.itk_bold_to_t1')]), | ||
(bold_t1_trans_wf, outputnode, [('outputnode.bold_t1', 'bold_t1')]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets combine all of the (bold_t1_trans_wf, outputnode,
connections.
Changes proposed in this pull request
As discussed with @effigies, splits the existing
bold_reg_wf
into two separate, smaller workflows, one of which calculates the registrations and the other of which applies the result in a single-shot. This will make integration of ME-EPI workflow changes possible.Documentation that should be reviewed
Updated docstrings for both of the new functions.