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

ENH: Implement Jacobian weighting during unwarp #391

Merged
merged 1 commit into from
Oct 2, 2023
Merged

Conversation

effigies
Copy link
Member

@effigies effigies commented Aug 18, 2023

This turned out to be dead simple. The basic idea of a Jacobian is to map volumes in the target space to volumes in the source space. We have a particularly easy case, because there is a warp only in one dimension, so our Jacobian is trivially equal to $1 + dx/dP$ where $P$ is the phase-encoding axis.

It seems likely that we will need to adjust this when permitting other transformations. While affine transforms will scale all points uniformly (and can thus be ignored for unitless values like BOLD), warps to template will almost certainly produce additional changes worth accounting for, though the scale of those effects is probably small compared to SDC.

Closes #382.

@effigies

This comment was marked as outdated.

@effigies
Copy link
Member Author

effigies commented Aug 21, 2023

Hmm. I wonder if I should be scaling the jacobian by the voxel volume, not from 0-1... (This was not even wrong.)

@effigies
Copy link
Member Author

Continuing to work on this. I have attempted to create a Jacobian determinant in a couple ways, none of which significantly affected my use case. I believe that the mask used to fit the spline field is the more significant problem for phasediff fieldmaps. I think we need to mask the output field.

@effigies effigies marked this pull request as draft September 27, 2023 14:54
@effigies
Copy link
Member Author

effigies commented Sep 27, 2023

As discussed with @oesteban, the shortest path forward here is to pull the TOPUP-generated Jacobians to the application stage. Calculating our own Jacobians can wait.

Turns out trying to make use of TOPUPs Jacobians was more pain than finally wrapping my head around exactly what a Jacobian is in this context.

@effigies effigies changed the base branch from maint/scipy-cubic-deprecation-2 to master October 1, 2023 04:32
@codecov
Copy link

codecov bot commented Oct 1, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (50ace2a) 80.57% compared to head (c8a4f3d) 80.59%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #391      +/-   ##
==========================================
+ Coverage   80.57%   80.59%   +0.01%     
==========================================
  Files          26       26              
  Lines        2291     2293       +2     
  Branches      287      287              
==========================================
+ Hits         1846     1848       +2     
  Misses        401      401              
  Partials       44       44              
Flag Coverage Δ
unittests ∅ <ø> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
sdcflows/transform.py 77.90% <100.00%> (+0.24%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@effigies
Copy link
Member Author

effigies commented Oct 1, 2023

applytopup-corrected

Screenshot from 2023-10-01 00-33-32

sdcflows @ master-corrected

Screenshot from 2023-10-01 00-33-58

With this PR

Screenshot from 2023-10-01 00-34-08

It is not 100% identical, and this may be a result of premature coercion to float32 of coordinates during resampling, or it could be our spline knots are very slightly differently defined.

@oesteban
Copy link
Member

oesteban commented Oct 1, 2023 via email

@effigies
Copy link
Member Author

effigies commented Oct 2, 2023

@oesteban or @mgxd would you care to review? (Feel free to say you don't have time but I should/shouldn't go ahead and merge.)

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.

this is great and lgtm - let's get it into dev for further testing

@effigies effigies merged commit 3e318c5 into master Oct 2, 2023
6 checks passed
@effigies effigies deleted the enh/jacobian branch October 2, 2023 14:18
effigies added a commit to effigies/sdcflows that referenced this pull request Oct 4, 2023
effigies added a commit to effigies/sdcflows that referenced this pull request Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Jacobian attenuation in the target space
3 participants