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: Mask multi-echo data with reference mask instead of echo-specific masks #2349

Merged
merged 8 commits into from
Sep 5, 2021

Conversation

tsalo
Copy link
Collaborator

@tsalo tsalo commented Dec 17, 2020

Closes #2348.

Changes proposed in this pull request

  • Instead of skull-stripping each echo-specific output of bold_bold_trans_wf before calling bold_t2s_wf, use the reference image-based mask from bold_bold_trans_wf .
  • Add bold_mask input to init_bold_t2s_wf.
  • Add mask_file input to T2SMap interface.

Documentation that should be reviewed

None

I didn't realize that the bold_bold_trans_wf included the mask as an output too. Still need to select the first one, although the mask should be the same across echoes.
fmriprep/workflows/bold/t2s.py Outdated Show resolved Hide resolved
(skullstrip_bold_wf, join_echos, [
('outputnode.skull_stripped_file', 'skullstripped_bold_files')]),
# use reference image mask used by bold_bold_trans_wf
(bold_bold_trans_wf, bold_t2s_wf, [
Copy link
Member

Choose a reason for hiding this comment

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

I'd be careful to ensure that bold_bold_trans_wf does not depend on bold_t2s_wf. If this assumption is acceptable, then this seems the most reasonable solution to this problem.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is that because it would introduce a loop in the workflow? I agree that T2* mapping/optimal combination should happen after the native-space transforms are applied, mostly so that those transforms can be done all at once. There are solid reasons to do optimal combination after distortion correction, but I don't think that will have a very large effect on the data and I doubt that it's worth the extra effort since it would involve splitting up the transforms performed by bold_bold_trans_wf into two sets.

Copy link
Member

Choose a reason for hiding this comment

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

Is that because it would introduce a loop in the workflow?

Yes, Nipype will not handle loops.

Co-authored-by: Oscar Esteban <code@oscaresteban.es>
@tsalo
Copy link
Collaborator Author

tsalo commented Dec 18, 2020

The failures seem like they're unrelated to the changes in this PR:

_______________ ERROR collecting fmriprep/interfaces/__init__.py _______________
/src/fmriprep/fmriprep/interfaces/__init__.py:5: in <module>
    from niworkflows.interfaces import (
/usr/local/miniconda/lib/python3.7/site-packages/niworkflows/interfaces/freesurfer.py:10: in <module>
    from skimage import morphology as sim
/usr/local/miniconda/lib/python3.7/site-packages/skimage/__init__.py:121: in <module>
    from ._shared import geometry
skimage/_shared/geometry.pyx:1: in init skimage._shared.geometry
    ???
E   ValueError: numpy.ufunc size changed, may indicate binary incompatibility. Expected 216 from C header, got 192 from PyObject

@oesteban
Copy link
Member

@tsalo could you rebase to see if the errors have been fixed?

@tsalo
Copy link
Collaborator Author

tsalo commented Apr 28, 2021

Done!

@effigies
Copy link
Member

effigies commented Sep 1, 2021

@tsalo Quick bump to re-rebase.

@tsalo
Copy link
Collaborator Author

tsalo commented Sep 3, 2021

@effigies done!

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.

This looks reasonable to me, and the ds210 outputs look sensible.

@effigies
Copy link
Member

effigies commented Sep 3, 2021

@oesteban @mgxd Any objection to sneaking this into 21.0?

@effigies effigies merged commit 10be2dc into nipreps:master Sep 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skull-strip first echo and apply mask to other echoes
4 participants