Skip to content
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

Merged
merged 7 commits into from Sep 15, 2018

Conversation

emdupre
Copy link
Collaborator

@emdupre emdupre commented Sep 12, 2018

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.

@emdupre emdupre changed the title [ENH] Split bold_reg_wf [WIP, RF] Split bold_reg_wf Sep 13, 2018
Copy link
Member

@effigies effigies left a 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.

@@ -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`
Copy link
Member

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.

Copy link
Member

@effigies effigies left a 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!

('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')]),
Copy link
Member

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.

(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')]),
Copy link
Member

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.

('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')]),
Copy link
Member

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.

@effigies effigies changed the title [WIP, RF] Split bold_reg_wf [RF] Split BOLD-T1w registration into calculation/application workflows Sep 15, 2018
@effigies effigies merged commit 250b6f0 into nipreps:master Sep 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants