-
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] Template-based masking of EPI boldrefs #1321
Conversation
The problem with echo 1 of ds216 seems to have been introduced here (this PR vs latest master) |
The weird result in https://2488-53608443-gh.circle-artifacts.com/0/tmp/data/reports/ds000216/sub-03_task-rest_echo-1_bold_masks.svg does not happen when I test locally (???) |
Seems like the weird problem on ds216 is now fixed. At this point I would ask: do you think it is maybe worth changing the whole algorithm to the following?:
|
Let's test on the subject from #1181, as well. |
Still testing on additional datasets, but this seems ready for review. |
Hi, @oesteban. I did a quick test using this PR on my previous failed data. Generally, it gives pretty good results. But I think there are several issues we need to address.
|
Thanks a lot for your precious feedback! @ZhifangYe
Could you provide more information about the error (full trace) or even the dataset (privately is fine) where this is happening? This is a new caching procedure we are testing and it does not surprise me you got this kind of problem.
Cool, sub-26_task-rest is now within the regression tests, so I'll work on this one particularly.
This question is also very pertinent. I'll dig up some partial FoV datasets from OpenNeuro and add them to the test battery. Thanks a lot for your input, very much appreciated! |
Ref #900 |
Here's the fMRIprep error log and the verbose log of N4 correction command. I think it's due to the very small discrepancy between the qform matrix of pre_mask and ref_image. The following link is the raw data to replicate this error. https://www.dropbox.com/s/akfyadrih5lgcbd/sub-30.zip?dl=0 |
Correct, there is a minimal difference in the qform. |
Alright, this ready for review (@ZhifangYe, @effigies). I've just fixed the issue of mismatched headers before N4. Since we don't make any promises about very limited FoV (https://github.com/poldracklab/fmriprep/labels/partial%20FOV), I propose to merge this in and come back later to the FoV issue. Regarding sub-26_task-rest, please check out the regression tests under the test_pytest build of Circle. |
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'm not sure about the new interface.
fmriprep/interfaces/images.py
Outdated
imghdr['dim_info'] = refhdr['dim_info'] # dim_info is lost sometimes | ||
|
||
# Set qform | ||
qform, qcode = refhdr.get_qform(coded=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.
Are all images guaranteed to have non-zero codes? If qcode == 0
, qform == None
.
fmriprep/interfaces/images.py
Outdated
imghdr.set_qform(qform, int(qcode)) | ||
|
||
# Set sform | ||
sform, scode = refhdr.get_sform(coded=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.
fmriprep/interfaces/images.py
Outdated
newpath=runtime.cwd) | ||
|
||
imgnii.__class__(imgnii.get_data(), imgnii.affine, imghdr).to_filename( | ||
out_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.
I don't think this will work how you want. You may want to add tests.
If imgnii.affine
does not match the result of imgnii.header.get_best_affine()
, it will be pushed into the header, probably as the sform. If you've just put refhdr.get_sform()
into the imghdr
sform matrix, then these are unlikely to match, and you'll be pushing the original affine right back in.
A better approach would be:
imgnii.__class__(imgnii.get_data(), imghdr.get_best_affine(), imghdr).to_filename(
out_file)
This is sub-26_task-rest: https://2591-53608443-gh.circle-artifacts.com/0/tmp/data/reports/ds001240/sub-26_task-rest_bold_masks.svg which looks okay. We are seeing ds005 fail due to a problem checking filenames that is also happening in master. I'm guessing that would be a trivial change to do in master. Thumbs up? |
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 looks good. I took another read through, and only saw one thing, which should have no impact one way or another, but would reduce the number of branches.
fmriprep/workflows/bold/util.py
Outdated
if pre_mask: | ||
workflow.connect([ | ||
(inputnode, enhance_and_skullstrip_bold_wf, [('bold_mask', 'inputnode.pre_mask')]), | ||
]) |
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 think this should be okay to set unconditionally.
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 test results look good to me. I'm really happy to see the issue is resolved. 👍
I read through the code and only find the doc in init_enhance_and_skullstrip_bold_wf
line 185 doesn't match. I think it should be '7. Calculate a final mask as the intersection of 4) and 6).'.
Changes proposed in this pull request
This PR explores template-based masking of boldrefs to initialize the skull-stripping workflow (see issues related on #1000).
This PR is also highly related to #1050.
Documentation that should be reviewed
This is WIP, will update documentation when it is proven to work out.