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

FIX: SDC-SyN ("fieldmap-less") overhaul #239

Merged
merged 16 commits into from
Oct 6, 2021
Merged

FIX: SDC-SyN ("fieldmap-less") overhaul #239

merged 16 commits into from
Oct 6, 2021

Conversation

oesteban
Copy link
Member

@oesteban oesteban commented Oct 1, 2021

In the context of nipreps/fmriprep#2530, this PR takes a deep revision on the SDC-SyN implementation. In passing, the problems about suboptimal registration (#85) are also addressed.

@oesteban oesteban linked an issue Oct 1, 2021 that may be closed by this pull request
@pep8speaks
Copy link

pep8speaks commented Oct 4, 2021

Hello @oesteban, Thank you for updating!

Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally, pip install flake8 and then run flake8 sdcflows.

Comment last updated at 2021-10-06 13:31:44 UTC

Salient points on this commit:

- There's no need for intensity inversion if MI is used as cost function
- Use of the T1w as reference (fixed) image increases reliability
- Giving more flexibility to ``restrict_deformation`` with 0.1 instead
  of 1 makes optimization faster leveraging a minimal increase of dofs
- To double-check things look good, use our internal transform object
  to generate the unwarped reference, and then generate the field, which
  should be fairly close to the one generated by ANTs.
- Addressed LPS orientation bug in the function node that extracts the
  nonzero component of distortion.
- Better registration parameters, with thanks to @yasseraleman for
  suggestions (in particular, the one about flexibilizing the gradient
  restriction above came from him).
- Use of laplacians in a multi-channel registration framework

Resolves: #186.
@oesteban oesteban force-pushed the fix/syn-revision branch 2 times, most recently from bfa2df9 to 816c894 Compare October 5, 2021 10:22
@oesteban oesteban marked this pull request as ready for review October 5, 2021 15:14
@oesteban
Copy link
Member Author

oesteban commented Oct 5, 2021

Okay, I think the registration parameters of both initialization and SyN steps, and the B-Spline approximation, are pretty good for the one subject I'm testing on from ds005. We probably want to merge this and maybe refine parameters as we test this on other datasets.

@oesteban oesteban requested review from effigies and mgxd October 5, 2021 15:42
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.

I think it makes sense. If I understand correctly, we're moving from histogram-based matching to finding a single deformation that optimally registers both intensity and the intensity-derived Laplacian (~edges) images?

Some comments (mostly readability) about the inverse functions.

sdcflows/tests/test_transform.py Outdated Show resolved Hide resolved
sdcflows/transform.py Outdated Show resolved Hide resolved
sdcflows/transform.py Outdated Show resolved Hide resolved
sdcflows/transform.py Outdated Show resolved Hide resolved
sdcflows/transform.py Outdated Show resolved Hide resolved
sdcflows/transform.py Outdated Show resolved Hide resolved
sdcflows/transform.py Outdated Show resolved Hide resolved
Copy link
Contributor

@mgxd mgxd left a comment

Choose a reason for hiding this comment

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

lgtm, minor comments

sdcflows/transform.py Outdated Show resolved Hide resolved
sdcflows/workflows/fit/syn.py Show resolved Hide resolved
sdcflows/workflows/fit/syn.py Outdated Show resolved Hide resolved
@oesteban
Copy link
Member Author

oesteban commented Oct 6, 2021

Thanks for the feedback :)

Two late additions:

  • 43712de where we make sure the restrict_deformation input of ANTs' registration is in RAS coordinates (instead of voxel coordinates, as in the BIDS specs).
  • d4b6567 where the documentation is extended

@effigies
Copy link
Member

effigies commented Oct 6, 2021

I believe the test failure is that sd_syn_sloppy.json hasn't been updated to match the number of levels of sd_syn.json.

"number_of_iterations": [ [ 10, 0 ], [ 10, 5, 0 ], [ 2 ] ],
"metric": [ ["Mattes", "Mattes"], ["Mattes", "Mattes"] ],
"metric_weight": [ [0.5, 0.5], [0.5, 0.5] ],
"number_of_iterations": [ [ 20, 10 ], [ 2 ] ],
Copy link
Member

Choose a reason for hiding this comment

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

Same as sd_syn, but with factor of 0.1 and 0.2 applied, matching the previous factors 0.1, 0.5, 0.2.

@oesteban
Copy link
Member Author

oesteban commented Oct 6, 2021

Okay, I think all the comments have been addressed - I'm going to press the red (green) button now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants