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,WIP: New transforms module #656

Closed
wants to merge 36 commits into from
Closed

Conversation

oesteban
Copy link
Contributor

@oesteban oesteban commented Aug 1, 2018

This PR adds a new nibabel.transforms module that implements geometric transforms over images and surfaces.

This is a prototype for discussion that implements affine transformations and a very suboptimal free-deformation field transform, over which other models can be built (e.g. a BSpline transform).

The vision for the module includes not only making it easy to resample images with the application of transforms, but also to read the most common transform formats from the most widely used tools (see #654 )

@oesteban
Copy link
Contributor Author

oesteban commented Aug 9, 2018

@satra, talking today with @effigies, he commented that you mentioned that warpings had very different formats across tools, which would make this idea almost impossible. Could you provide some details about that?

@satra
Copy link
Member

satra commented Aug 10, 2018

@oesteban - nothing is impossible, but this may take more work than one may be comfortable in taking on. i think the nonlinear transforms are more complicated than the affine ones, because the warps are represented in different ways in FSL/FNIRT, ANTs, SPM/Dartel, FreeSurfer/CVS, together with different coordinate systems. but some affine/linear transforms can be a little complicated as well (e.g., freesurfer xfms).

@jclauneuro
Copy link

Thanks for looking into building this functionality into nibabel. Just wanted to link this with a thread on Neurostars where @oesteban mentioned this PR specifically in the context of "disassembling"/"decomposing" composite (.h5) transforms.

@chrisgorgo
Copy link
Member

We had some degree of success converting between fsl and ants warps (in context of fieldmapless correction in FMRIPREP). I remain optimistic.

This will be an incredibly useful project in context of BIDS Derivatives.

@nibotmi
Copy link
Contributor

nibotmi commented Dec 31, 2018

☔ The latest upstream changes (presumably #700) made this pull request unmergeable. Please resolve the merge conflicts.

@effigies effigies added this to the 3.0.0 milestone May 28, 2019
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.

@oesteban I have a number of comments in the backlog I might as well submit. I haven't read the whole thing yet.

self._ndindex = None
self._coords = None
if self._ndim not in [2, 3]:
raise ValueError('Invalid image space (%d-D)' % self._ndim)
Copy link
Member

Choose a reason for hiding this comment

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

Realigning 4D series seems like a pretty common use case that you're precluding here.

Copy link
Member

Choose a reason for hiding this comment

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

I think re-reading this, it's actually just that the reference image has to be 2 or 3D. Still, there's no reason I couldn't pass in a 4D image; it's just all in the same 3D space.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I'll work on generalization later down the line.

'''
self._matrix = np.array(matrix)
assert self._matrix.ndim == 2, 'affine matrix should be 2D'
assert self._matrix.shape[0] == self._matrix.shape[1], 'affine matrix is not square'
Copy link
Member

Choose a reason for hiding this comment

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

ValueErrors are more appropriate here than AssertionErrors.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like this line changed since I wrote it, but still applies.

.gitignore Outdated Show resolved Hide resolved
nibabel/transform/__init__.py Outdated Show resolved Hide resolved
nibabel/transform/base.py Show resolved Hide resolved
nibabel/transform/base.py Outdated Show resolved Hide resolved
nibabel/transform/linear.py Outdated Show resolved Hide resolved
if coords.shape[0] == self._matrix[index].shape[0] - 1:
coords = np.append(coords, [1])
affine = self._matrix[index] if forward else np.linalg.inv(self._matrix[index])
return affine.dot(coords)[:-1]
Copy link
Member

Choose a reason for hiding this comment

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

Should this [:-1] be conditional on coords being appended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm pushing this to the backlog checks that need be done before merge. For now I'm just working on a very much needed rebase and implementing all the functionalities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I've just realized why this [:-1] is here - to drop the last element of the homogeneous coordinate, which is always a one.

@oesteban
Copy link
Contributor Author

@effigies would deepdish be an acceptable dependency?

@effigies
Copy link
Member

What's the advantage over h5py?

@codecov
Copy link

codecov bot commented Sep 19, 2019

Codecov Report

Merging #656 into master will decrease coverage by 2.11%.
The diff coverage is 25.13%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #656      +/-   ##
==========================================
- Coverage    90.1%   87.98%   -2.12%     
==========================================
  Files          96      100       +4     
  Lines       11910    12271     +361     
  Branches     2125     2181      +56     
==========================================
+ Hits        10731    10797      +66     
- Misses        834     1118     +284     
- Partials      345      356      +11
Impacted Files Coverage Δ
nibabel/__init__.py 89.47% <100%> (-3.39%) ⬇️
nibabel/affines.py 100% <100%> (ø) ⬆️
nibabel/transform/__init__.py 100% <100%> (ø)
nibabel/transform/linear.py 16.96% <16.96%> (ø)
nibabel/transform/nonlinear.py 22.91% <22.91%> (ø)
nibabel/volumeutils.py 92.23% <33.33%> (-0.36%) ⬇️
nibabel/transform/base.py 34.83% <34.83%> (ø)
nibabel/pydicom_compat.py 63.63% <0%> (-36.37%) ⬇️
nibabel/environment.py 75% <0%> (-20%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 370b50f...596994c. Read the comment docs.

This implementation mimics the implementation of AFNI.
@oesteban
Copy link
Contributor Author

@effigies are we using pytest? any chance we could migrate to it?

@effigies
Copy link
Member

We're using nose. Moving to pytest would be a significant effort.

@oesteban
Copy link
Contributor Author

Noting that I'm following discussions for the implementation of AFNI affines here - afni/afni#116

matthew-brett added a commit that referenced this pull request Oct 15, 2019
MRG: Add a utility to calculate obliquity of affines

This implementation mimics the implementation of AFNI, and will be useful in #656 .
@effigies effigies removed this from the 3.0.0 RC1 milestone Oct 28, 2019
@iancharest
Copy link

Hi Guys, thanks all for your great efforts with nibabel. I have been following this PR closely as it would be very useful to ease many ongoing projects we have. Out of curiosity, was there any reason why this PR was removed from the 3.0.0 RC1 milestone? Cheers!

@effigies
Copy link
Member

Mostly that it didn't look like this was going to be finished in time for that release.

This work has been moved to poldracklab/nitransforms for now. The pace of development can be quicker over there. When that stabilizes, we can work out whether and how to include it into nibabel.

@oesteban
Copy link
Contributor Author

oesteban commented Oct 31, 2019 via email

@oesteban
Copy link
Contributor Author

I'm going to close this PR. To follow up, please check https://github.com/poldracklab/nitransforms

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.

7 participants