-
Notifications
You must be signed in to change notification settings - Fork 27
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: Base implementation for phase1/2 fieldmaps #60
Conversation
Hello @oesteban, Thank you for updating! Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally, Comment last updated at 2019-11-25 18:15:45 UTC |
1198fa5
to
bb08171
Compare
Please have a look @Aksoo 👍 |
bb08171
to
f13eeef
Compare
Fixing the ``acq`` -> ``acquisition`` query made the HCP test case break.
f13eeef
to
ea0cf68
Compare
I think the main differences between the phasediff and the phase1/2 now are derived from the quite different mask of the magnitude image: Any voices against merging this @Aksoo, @mattcieslak ? |
Do you have the actual fieldmaps that we can look at? I'm only seeing the figures on circleci |
Okay, we should now have access to the corresponding NIfTI files resulting from the tests on CircleCI: https://app.circleci.com/jobs/github/oesteban/sdcflows/303/artifacts They will be available as soon as the tests finish. I will give you a heads-up since I'm very interested in getting this merged ASAP (we are holding a release of fMRIPrep for this and some other changes). |
@mattcieslak - results are up (https://app.circleci.com/jobs/github/oesteban/sdcflows/303/artifacts). LMKWYT |
These look very good. The actual values line up closely in the phasediff and subtracted phases |
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 follows the FUGUE guide, with unwrapping the phase images, then subtracting them.
Thanks for the review! Very happy to improve this base implementation with your ideas and @Aksoo's. Let's roll this out and build from there. |
Late response (punted and forgot about this when GitHub broke yesterday) but everything looks good as far as following the old protocol. The refactors greatly improved code organization as well. I will follow up with my suggestion in #59. |
Putting together the lessons learned in #30, leveraging #52 and #53 (unfolded from #30 too), and utilizing #50 and #51, this workflow adds the phase difference map calculation, considering it one use-case of the general phase-difference fieldmap workflow.
On top of this PR, we can continue the discussions held in #30. Probably, we will want to address #23 the first - the magnitude segmentation is sometimes really bad (e.g. see the phase1/2 unit test).
Another discussion arisen in #30 is the spatial smoothing of the fieldmap (#22).
Finally, the plan is to revise this implementation and determine whether the subtraction should happen before or after PRELUDE, and whether the arctan2 route is more interesting (#59).