RF: Move reorientation into image classes #544
Conversation
Codecov Report
@@ Coverage Diff @@
## master #544 +/- ##
==========================================
+ Coverage 94.26% 94.27% +0.01%
==========================================
Files 177 177
Lines 24300 24333 +33
Branches 2608 2610 +2
==========================================
+ Hits 22906 22941 +35
+ Misses 919 918 -1
+ Partials 475 474 -1
Continue to review full report at Codecov.
|
|
Can't request a review, but @markhymers, if you have any feedback, please jump in. |
There was a problem hiding this comment.
Looks good in general, but the name transpose still makes me suffer. It is not only that this transpose has a different meaning from np.transpose, but also that the transformation can also be an axis flip. Can you think of anything better than the current options?
nibabel/nifti1.py
Outdated
| def transpose(self, ornt): | ||
| """Apply an orientation change and return a new image | ||
|
|
||
| If image already has orientation, return the original image, unchanged |
There was a problem hiding this comment.
I guess you mean, if ornt is the identity?
There was a problem hiding this comment.
You're right. I wrote that bit while I thought ornt was the target orientation, not the transformation.
nibabel/spatialimages.py
Outdated
| def transpose(self, ornt): | ||
| """Apply an orientation change and return a new image | ||
|
|
||
| If image already has orientation, return the original image, unchanged |
|
I agree transpose isn't great. What about |
effigies
force-pushed
the
improve_as_closest_canonical
branch
from
6448853 to
7e8c4b8
Compare
|
How do you feel about |
|
Fine by me. |
|
Then fine by me too. Merge when tests pass? |
effigies
force-pushed
the
improve_as_closest_canonical
branch
from
7e8c4b8 to
99a7e85
Compare
|
Sounds good. I'll add a changelog entry, as well. |
This avoids make failing to cause a rebuild when files are changed Signed-off-by: Mark Hymers <mark.hymers@ynic.york.ac.uk>
At present, we leave misleading information in dim_info when using as_closest_canonical. This patch makes us update the information based on our axis re-ordering and adds tests that it has been done. Signed-off-by: Mark Hymers <mark.hymers@ynic.york.ac.uk>
Use of the set_dim_info function showed up a FutureWarning error in python3. python3 now really dislikes comparing to None using ==, which is what "in" does. Work around this to silence the warning. Signed-off-by: Mark Hymers <mark.hymers@ynic.york.ac.uk>
Signed-off-by: Mark Hymers <mark.hymers@ynic.york.ac.uk>
As per Matthew's comments, place the logic which re-orients the data into a function in SpatialImage and override this in Nifti1Pair with a version which updates dim_info. Also add new test cases using AnalyzeImage so that we cover both possibilities. Signed-off-by: Mark Hymers <mark.hymers@ynic.york.ac.uk>
Signed-off-by: Mark Hymers <mark.hymers@ynic.york.ac.uk>
Return original image if no transform is to be made
effigies
force-pushed
the
improve_as_closest_canonical
branch
from
70a7994 to
e6de59d
Compare
#526 seems to have stalled. I'd like to incorporate these changes, as well as those discussed by @matthew-brett and myself.
On renaming
transposetoas_oriented, while I agree that distinguishing fromnp.transposemay be desirable, I disagree that this would be a more appropriate name. That is, theorntmatrix doesn't describe an orientation, but a transformation. I would expectas_orientedto be idempotent for a given orientation, which it is not. Withornt = [[0, -1], [2, 1], [1, 1]], I could flip back and forth betweenRASandLSAall day long.I also moved the identity transform check into
transpose, and letNifti1Pair.transposecallSpatialImage.transpose, which I think helps clarify which parts of the operation are specific to NIfTIs.Closes #526.