-
Notifications
You must be signed in to change notification settings - Fork 297
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: Add option to exclude projecting high variance voxels to surface #2855
Conversation
…mmits) Squashed commits: [b1597c2] output goodvoxels-masked BOLD surfs in bold_surf_wf
…n a separate branch)
… separate branch)
Thanks for opening this pull request! It looks like this is your first time contributing to fMRIPrep. 😄 "name": "Contributor, New FMRIPrep",
"affiliation": "Department of fMRI prep'ing, Open Science Made-Up University",
"orcid": "<your id>"
}, ```
Of course, if you want to opt-out this time there is no problem at all with adding your name later. You will be always welcome to add it in the future whenever you feel it should be listed. |
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.
Some initial thoughts. This is still marked draft. Are you planning on doing more?
@@ -488,6 +489,13 @@ surface, is sampled at 6 intervals and averaged. | |||
|
|||
Surfaces are generated for the "subject native" surface, as well as transformed to the | |||
``fsaverage`` template space. | |||
|
|||
The flag ``--project_goodvoxels``, when enabled, excludes voxels whose timeseries have |
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.
The flag ``--project_goodvoxels``, when enabled, excludes voxels whose timeseries have | |
The flag ``--project-goodvoxels``, when enabled, excludes voxels whose timeseries have |
fmriprep/interfaces/volume.py
Outdated
surface = File( | ||
exists=True, | ||
mandatory=True, | ||
argstr="%s ", |
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.
argstr="%s ", | |
argstr="%s", |
fmriprep/interfaces/volume.py
Outdated
surface = File( | ||
exists=True, | ||
mandatory=True, | ||
argstr="%s ", | ||
position=0, | ||
desc="the input surface", |
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.
A couple things:
- Nipype convention to use
_file
to indicate files. - No need for an extra space in argstr.
- I think
connectome_wb
is pretty finicky about the type and extension, so we can try to indicate it in the doc.
surface = File( | |
exists=True, | |
mandatory=True, | |
argstr="%s ", | |
position=0, | |
desc="the input surface", | |
surf_file = File( | |
exists=True, | |
mandatory=True, | |
argstr="%s", | |
position=0, | |
desc="Input surface GIFTI file (.surf.gii)", |
fmriprep/interfaces/volume.py
Outdated
ref_space = File( | ||
exists=True, | ||
mandatory=True, | ||
argstr="%s ", | ||
position=1, | ||
desc="a volume in the desired output space (dims, spacing, origin)", | ||
) |
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.
ref_space = File( | |
exists=True, | |
mandatory=True, | |
argstr="%s ", | |
position=1, | |
desc="a volume in the desired output space (dims, spacing, origin)", | |
) | |
ref_file = File( | |
exists=True, | |
mandatory=True, | |
argstr="%s", | |
position=1, | |
desc="NIfTI volume in the desired output space (dims, spacing, origin)", | |
) |
fmriprep/interfaces/volume.py
Outdated
out_vol = File( | ||
name_source=["surface"], | ||
name_template="%s.distvol.nii.gz", | ||
argstr="%s ", | ||
position=2, | ||
desc="output - the output volume", | ||
) |
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 generally use _
to separate a descriptive tag, rather than a .
:
out_vol = File( | |
name_source=["surface"], | |
name_template="%s.distvol.nii.gz", | |
argstr="%s ", | |
position=2, | |
desc="output - the output volume", | |
) | |
out_file = File( | |
name_source=["surface"], | |
name_template="%s_distvol.nii.gz", | |
argstr="%s", | |
position=2, | |
desc="Name of output volume containing signed distances", | |
) |
fmriprep/interfaces/volume.py
Outdated
argstr="-winding %s ", | ||
usedefault=True, | ||
position=8, | ||
desc="winding method for point inside surface test, choices: " | ||
"EVEN_ODD (default) " | ||
"NEGATIVE " | ||
"NONZERO " | ||
"NORMALS " |
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.
argstr="-winding %s ", | |
usedefault=True, | |
position=8, | |
desc="winding method for point inside surface test, choices: " | |
"EVEN_ODD (default) " | |
"NEGATIVE " | |
"NONZERO " | |
"NORMALS " | |
argstr="-winding %s", | |
usedefault=True, | |
desc="winding method for point inside surface test" |
fmriprep/interfaces/volume.py
Outdated
out_vol = File(exists=True, desc="output - the output volume") | ||
roi_out = File(desc="output roi volume of where the output has a computed value") |
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.
exists=True
is asking for trouble: nipy/nipype#3493
out_vol = File(exists=True, desc="output - the output volume") | |
roi_out = File(desc="output roi volume of where the output has a computed value") | |
out_file = File(desc="Name of output volume containing signed distances") | |
out_mask = File(desc="Name of file to store a mask where the ``out_file`` has a computed value") |
fmriprep/interfaces/volume.py
Outdated
""" | ||
CREATE SIGNED DISTANCE VOLUME FROM SURFACE |
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.
""" | |
CREATE SIGNED DISTANCE VOLUME FROM SURFACE | |
"""CREATE SIGNED DISTANCE VOLUME FROM SURFACE | |
fmriprep/interfaces/volume.py
Outdated
|
||
def _list_outputs(self): | ||
outputs = super(CreateSignedDistanceVolume, self)._list_outputs() | ||
return outputs |
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.
def _list_outputs(self): | |
outputs = super(CreateSignedDistanceVolume, self)._list_outputs() | |
return outputs |
# 0, 1 = wm; 2, 3 = pial; 6, 7 = mid | ||
# note that order of lh / rh within each surf type is not guaranteed due to use | ||
# of unsorted glob by FreeSurferSource prior, but we can do a sort with | ||
# _sorted_by_basename to ensure consistent ordering | ||
select_wm = pe.Node( | ||
niu.Select(index=[0, 1]), | ||
name="select_wm", | ||
mem_gb=DEFAULT_MEMORY_MIN_GB, | ||
) | ||
|
||
select_pial = pe.Node( | ||
niu.Select(index=[2, 3]), | ||
name="select_pial", | ||
mem_gb=DEFAULT_MEMORY_MIN_GB, | ||
) | ||
|
||
select_midthick = pe.Node( | ||
niu.Select(index=[6, 7]), | ||
name="select_midthick", | ||
mem_gb=DEFAULT_MEMORY_MIN_GB, | ||
) | ||
|
||
create_wm_distvol = pe.MapNode( | ||
CreateSignedDistanceVolume(), | ||
iterfield=["surface"], |
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 really be its own workflow at this point. In this workflow, we could then have a small adaptation:
if project_goodvoxels:
ribbon_mask_wf = init_ribbon_mask_wf()
apply_ribbon_mask = pe.Node(
fsl.ApplyMask(),
name_source=['in_file'],
name="apply_goodvoxels_ribbon_mask",
mem_gb=mem_gb * 3,
)
wf.connect([
(inputnode, ribbon_mask_wf, [('anat_giftis', 'anat_giftis'),
('t1w_mask', 't1w_mask')]),
(rename_src, apply_ribbon_mask, [('out_file', 'in_file')]),
(ribbon_mask_wf, apply_ribbon_mask, [('mask_file', 'mask_file')]),
(apply_ribbon_mask, sampler, [('out_file', 'in_file')]),
])
else:
wf.connect([
(rename_src, sampler, [('out_file', 'in_file')]),
])
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.
My initial review on nibabies overlapped a lot with Chris's comments, so only ported over novel points - There are a lot of styling changes that hopefully will be addressed once #2857 is merged.
select_wm = pe.Node( | ||
niu.Select(index=[0, 1]), | ||
name="select_wm", | ||
mem_gb=DEFAULT_MEMORY_MIN_GB, |
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.
Generally, we can run most nipype utility interfaces on the master process since they are not intense operations - no need to potentially submit to a job queue.
mem_gb=DEFAULT_MEMORY_MIN_GB, | |
mem_gb=DEFAULT_MEMORY_MIN_GB, | |
run_without_submitting=True, |
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.
It might be worth taking the opportunity to rewrite some of this workflow instead of setting up a long fslmaths chain. Just a quick sketch, below. Happy to discuss at a convenient time.
thresh_wm_distvol = pe.MapNode( | ||
fsl.maths.MathsCommand(args="-thr 0 -bin -mul 255"), | ||
iterfield=["in_file"], | ||
name="thresh_wm_distvol", | ||
mem_gb=DEFAULT_MEMORY_MIN_GB, | ||
) | ||
|
||
uthresh_pial_distvol = pe.MapNode( | ||
fsl.maths.MathsCommand(args="-uthr 0 -abs -bin -mul 255"), | ||
iterfield=["in_file"], | ||
name="uthresh_pial_distvol", | ||
mem_gb=DEFAULT_MEMORY_MIN_GB, | ||
) | ||
|
||
bin_wm_distvol = pe.MapNode( | ||
fsl.maths.UnaryMaths(operation="bin"), | ||
iterfield=["in_file"], | ||
name="bin_wm_distvol", | ||
mem_gb=DEFAULT_MEMORY_MIN_GB, | ||
) | ||
|
||
bin_pial_distvol = pe.MapNode( | ||
fsl.maths.UnaryMaths(operation="bin"), | ||
iterfield=["in_file"], | ||
name="bin_pial_distvol", | ||
mem_gb=DEFAULT_MEMORY_MIN_GB, | ||
) | ||
|
||
split_wm_distvol = pe.Node( | ||
niu.Split(splits=[1, 1]), | ||
name="split_wm_distvol", | ||
mem_gb=DEFAULT_MEMORY_MIN_GB, | ||
) | ||
|
||
merge_wm_distvol_no_flatten = pe.Node( | ||
niu.Merge(2), | ||
no_flatten=True, | ||
name="merge_wm_distvol_no_flatten", | ||
mem_gb=DEFAULT_MEMORY_MIN_GB, | ||
) | ||
|
||
make_ribbon_vol = pe.MapNode( | ||
fsl.maths.MultiImageMaths(op_string="-mas %s -mul 255 "), | ||
iterfield=["in_file", "operand_files"], | ||
name="make_ribbon_vol", | ||
mem_gb=DEFAULT_MEMORY_MIN_GB, | ||
) | ||
|
||
bin_ribbon_vol = pe.MapNode( | ||
fsl.maths.UnaryMaths(operation="bin"), | ||
iterfield=["in_file"], | ||
name="bin_ribbon_vol", | ||
mem_gb=DEFAULT_MEMORY_MIN_GB, | ||
) | ||
|
||
split_squeeze_ribbon_vol = pe.Node( | ||
niu.Split(splits=[1, 1], squeeze=True), | ||
name="split_squeeze_ribbon", | ||
mem_gb=DEFAULT_MEMORY_MIN_GB, | ||
) | ||
|
||
combine_ribbon_vol_hemis = pe.Node( | ||
fsl.maths.BinaryMaths(operation="add"), | ||
name="combine_ribbon_vol_hemis", | ||
mem_gb=DEFAULT_MEMORY_MIN_GB, | ||
) |
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 suspect this workflow could be made more understandable with a couple pure Python interfaces. For instance, I think we could replace all of this with one function:
def make_ribbon(pial_distances, white_distances):
dist_lp = nb.load(pial_distances[0])
dist_rp = nb.load(pial_distances[1])
dist_lw = nb.load(white_distances[0])
dist_rw = nb.load(white_distances[1])
lmask = (dist_lw.get_fdata(dtype='f4') > 0) & (dist_lp.get_fdata(dtype='f4') < 0)
rmask = (dist_rw.get_fdata(dtype='f4') > 0) & (dist_rp.get_fdata(dtype='f4') < 0)
mask = np.uint8(lmask | rmask)
mask_img = nb.Nifti1Image(mask, dist_lp.affine, dist_lp.header)
mask_img.set_data_dtype(np.uint8)
return mask_img
Functions can then be promoted into interfaces with:
class MakeRibbon(SimpleInterface):
input_spec = ...
output_spec = ...
def _run_interface(self, runtime):
mask_img = make_ribbon(self.inputs.pial_distances, self.inputs.white_distances)
mask_img.to_filename("ribbon.nii.gz")
self._outputs["out_file"] = str(Path(runtime.cwd) / "ribbon.nii.gz")
return runtime
- Implemented the formatting suggestions by Mathias at the URLs below: - nipreps#235 - nipreps/fmriprep#2855 - Split the 3 workflow-connect blocks in the project-goodvoxels section of resampling.py into 3 different functions: ribbon, outliers, and vol-to-surf - Incomplete comments in resampling.py
This needs rebasing/merging with master. @madisoth Otherwise this is just waiting on templateflow/tpl-fsaverage#2? |
FIX: Minor anat wf & workbench.py fixes (+5 squashed commit) Squashed commit: [e995ec4] RF: Moved ribbon wf (from bold to anat) and reformatted [9676f14] Updated Docker version tag [532875e] STY: black and isort [946dc7e] Formatting fixes and resampling.py modularization - Implemented the formatting suggestions by Mathias at the URLs below: - nipreps#235 - nipreps/fmriprep#2855 - Split the 3 workflow-connect blocks in the project-goodvoxels section of resampling.py into 3 different functions: ribbon, outliers, and vol-to-surf - Incomplete comments in resampling.py [114f982] Modularized goodvoxels code in resampling.py Removed duplicate code by abstracting it into functions, clarifying the goodvoxels logic
… (update of #2855) (#2956) Adds option --project-goodvoxels to address #2822. If enabled, voxels whose timeseries have locally high coefficient of variation, or which lie outside the cortex (based on signed distance volumes generated from the WM and pial surfaces), are excluded from volume-to-surface sampling of BOLD timeseries. Not enabled by default. - adds MetricDilate interface for Workbench's wb_command -metric-dilate, which, when using the --project-goodvoxels option, fills zero-value vertices in the BOLD surface timeseries by dilating in data from the nearest "good" (nonzero, nonempty) vertex.
Changes proposed in this pull request
Adds option
--project-goodvoxels
to address #2822 .If enabled, voxels whose timeseries have locally high coefficient of variation, or which lie outside the cortex (based on signed distance volumes generated from the WM and pial surfaces), are excluded from volume-to-surface sampling of BOLD timeseries.
Note that unlike HCP / ciftify, no dilation or spatial smoothing is applied to the surface-resampled BOLD timeseries.
Documentation that should be reviewed