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

ENH: Better integration of *SDCFlows*' unwarping #2576

Merged
merged 1 commit into from
Oct 7, 2021

Conversation

oesteban
Copy link
Member

@oesteban oesteban commented Oct 6, 2021

This PR builds on top of #2547, unifying the final BOLDref computation.

Changes proposed in this pull request

Documentation that should be reviewed

@oesteban oesteban requested review from effigies and mgxd October 6, 2021 15:40
This PR builds on top of #2547, unifying the final BOLDref computation.
@@ -524,6 +524,15 @@ def init_func_preproc_wf(bold_file, has_fieldmap=False):
niu.IdentityInterface(fields=["bold", "boldref", "mask"]), name="bold_final"
)

# Generate a final BOLD reference
# This BOLD references *does not use* single-band reference images.
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, missed this in a previous PR. final_boldref_wf outputs to bold_final, which provides the reference image for bold_reg_wf. This is the primary use case for sbrefs, with the superior gray/white contrast. What are we now using sbrefs for?

Copy link
Member Author

Choose a reason for hiding this comment

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

What are we now using sbrefs for?

TBH, I don't know. I've been meaning to address #1511 for a long while, and the SDCFlows integration has made the SBRef path more complicated.

That said, my impression is that #1511 is becoming really urgent after the integration. Maybe we should just push the priority of that issue up?

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.

The changes in this PR seem correct. The dropping of sbrefs seems concerning, though that happened in a previous PR.

@oesteban
Copy link
Member Author

oesteban commented Oct 7, 2021

The dropping of sbrefs seems concerning, though that happened in a previous PR.

Yup - we were commenting right at the same time. See #2576 (comment)

@effigies
Copy link
Member

effigies commented Oct 7, 2021

Okay, let's merge and address #1511 asap.

@oesteban oesteban merged commit c50c3e6 into master Oct 7, 2021
@oesteban oesteban deleted the enh/post-sdc-boldref branch October 7, 2021 13:33
oesteban added a commit that referenced this pull request Oct 20, 2021
This connection can (should) be shared by both the SE and the ME paths.
That said, SDCFlows needs to be revised (nipreps/sdcflows#245) so that
fieldmaps without HMC are generated (and remove that ``_pop`` from this
connection).

Ammends: #2576.
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