-
Notifications
You must be signed in to change notification settings - Fork 260
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
Conversation
@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). |
Thanks for looking into building this functionality into |
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. |
☔ The latest upstream changes (presumably #700) made this pull request unmergeable. Please resolve the merge conflicts. |
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.
@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) |
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.
Realigning 4D series seems like a pretty common use case that you're precluding here.
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 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.
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.
Yep, I'll work on generalization later down the line.
nibabel/transform/base.py
Outdated
''' | ||
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' |
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.
ValueError
s are more appropriate here than AssertionError
s.
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.
Looks like this line changed since I wrote it, but still applies.
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] |
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.
Should this [:-1]
be conditional on coords being appended?
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 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.
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.
Okay, I've just realized why this [:-1]
is here - to drop the last element of the homogeneous coordinate, which is always a one.
What's the advantage over h5py? |
f11675c
to
f34affc
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
This implementation mimics the implementation of AFNI.
a529432
to
77de318
Compare
@effigies are we using pytest? any chance we could migrate to it? |
We're using nose. Moving to pytest would be a significant effort. |
…of linear and nonlinear transforms
…and without FixedParameters
…o N transforms (through time axis)
Added some initial tests to check the writing of transforms to ITK and FSL formats.
8fc8227
to
596994c
Compare
Noting that I'm following discussions for the implementation of AFNI affines here - afni/afni#116 |
MRG: Add a utility to calculate obliquity of affines This implementation mimics the implementation of AFNI, and will be useful in #656 .
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! |
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. |
Hi Ian, you might be interested in reading this https://osf.io/8aq7b
…On Thu, Oct 31, 2019, 04:25 Chris Markiewicz ***@***.***> wrote:
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.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#656?email_source=notifications&email_token=AAESDRTT7WCEN7DU2WY7YVLQRK6C3A5CNFSM4FNNVYSKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECXMPTI#issuecomment-548325325>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAESDRQSC4DFW3VQBFFNQZLQRK6C3ANCNFSM4FNNVYSA>
.
|
I'm going to close this PR. To follow up, please check https://github.com/poldracklab/nitransforms |
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 )