-
Notifications
You must be signed in to change notification settings - Fork 294
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
FIX: Threshold boldref resampled values to be non-negative #2630
Conversation
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.
Looks good in principle, although we may have this interface already in niworkflows (or something really close) and we may have a patched version of antsApplyTransforms that runs this thresholding too - worth having a look and confirming I'm wrong before merging.
@@ -620,7 +628,8 @@ def init_bold_preproc_trans_wf( | |||
(("bold_file", _first), "reference_image")]), | |||
(inputnode, merge, [("name_source", "header_source")]), | |||
(merge_xforms, bold_transform, [("out", "transforms")]), | |||
(bold_transform, merge, [("out_files", "in_files")]), | |||
(bold_transform, threshold, [("out_files", "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.
This would actually threshold the "preprocessed" echos, right? (which is actually the fix to the problem, as the boldref is not the target here).
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.
Yes, this thresholds the preprocessed echos (or the preprocessed BOLD, if single echo).
4cca38f
to
8e1099a
Compare
54945b9
to
2de37ac
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.
LGTM, just need to add in the missing imports
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.
Actually, that may have been pre-emptive...
0be6bb8
to
71310ef
Compare
71310ef
to
8af3d6b
Compare
https://neurostars.org/t/negative-data-following-multi-echo-optimal-combination-in-fmriprep/20698
The simplest place to do this seemed to be post-resample, pre-merge. This way it applies to single-echo and multi-echo data identically, with no possibility of missed branches during workflow construction.
I chose to make a simple Python interface rather than use fslmaths just because I didn't want to reason about possible type coercion.
Should we be doing this in all resampling workflows?