-
Notifications
You must be signed in to change notification settings - Fork 261
FIX: Correctly reorient dim_info when reorienting NIfTI images #741
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
Conversation
0e106e9
to
c13756d
Compare
Anybody up for a review on this? |
@markhymers This was originally your contribution. Would you care to check my logic, and make sure that I'm not breaking things? |
Codecov Report
@@ Coverage Diff @@
## master #741 +/- ##
==========================================
- Coverage 89.55% 89.54% -0.01%
==========================================
Files 93 93
Lines 11474 11470 -4
Branches 1992 1991 -1
==========================================
- Hits 10275 10271 -4
+ Misses 859 858 -1
- Partials 340 341 +1
Continue to review full report at Codecov.
|
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.
This commit does the same thing in a more pythonic way, as you note, with the list comprehension. It does not fix the bug.
Thanks for the review. I thought the confusion was that the freq/phase/slice ordering will only sometimes rotate in the same way as the shapes, but it turns out that orientations also didn't work how I thought... The issue was that I keep thinking of the orientation produced by axcodes2ornt as how to get from RAS to a given axis code, but it's exactly the reverse. So my consistency check was off, which was why your fix didn't seem to work. By instead cycling through orientations and getting the axis codes from the affine, I can now verify your proposed fix was correct. |
7847628
to
317b4ee
Compare
@constracti I found 46476cc in your history, so I went ahead and cherry-picked it and got rid of my flailing about for a wrong solution. I did still tack on the refactor to a list comprehension and casting to |
317b4ee
to
7c37276
Compare
Any more comments? |
Thanks for the contribution and feedback. |
Starting with a test to demonstrate the failure. Will push a fix shortly.
cc @constracti
Fixes #716.