Skip to content

Fix nibabel.orientations.io_orientation#1461

Closed
dkuegler wants to merge 3 commits intonipy:masterfrom
dkuegler:feature/io_orientation
Closed

Fix nibabel.orientations.io_orientation#1461
dkuegler wants to merge 3 commits intonipy:masterfrom
dkuegler:feature/io_orientation

Conversation

@dkuegler
Copy link

@dkuegler dkuegler commented Feb 2, 2026

This PR fixes the behavior of orientations.io_orientation, which extracts the ornt-array from an affine. Thus far, it always prioritizes the primary direction of the first column vector over other more aligned column vectors of the affine. Now, it ranks how well the axes align with the coordinate axes and iteratively selects the most aligned axis, eliminating that axis from further consideration. Currently, there are cases where the order of the column vectors impact results, now, changing the order of column vectors is a stable operation.

E.g. previously, an affine might have returned ASL, but after "singular" as_closest_canonical alignment does not return RAS (because of this instability).

This solution fixes several problems in my code, and would impact other functions using io_orientation such as as_closest_canonical and others (many indirectly).

I described the issue in #1460 as well.

This Commit fixes the behavior of orientations.io_orientation, which extracts the `ornt`-array from an affine.
Thus far, it always prioritizes the primary direction of the first column vector over other more aligned column vectors of the affine.
Now, it ranks how well the axes align with the coordinate axes and iteratively selects the most aligned axis, eliminating that axis from further consideration.
Currently, there are cases where the order of the column vectors impact results, now, changing the order of column vectors is a stable operation.

E.g. previously, an affine might have returned ASL, but after "singular" as_closest_canonical alignment does not return RAS (because of this instability).
@codecov
Copy link

codecov bot commented Feb 2, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.42%. Comparing base (56e6d92) to head (5efb537).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1461      +/-   ##
==========================================
- Coverage   95.42%   95.42%   -0.01%     
==========================================
  Files         209      209              
  Lines       29822    29818       -4     
  Branches     4483     4482       -1     
==========================================
- Hits        28457    28453       -4     
  Misses        930      930              
  Partials      435      435              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dkuegler
Copy link
Author

dkuegler commented Feb 3, 2026

Unfortunately, I copied the code from the wrong file and only had the untested version...
--> I fixed it now :)

Here is my test

@pytest.mark.parametrize(
    argnames=["affine", "axcode"],
    argvalues=[
        [np.diag([2, 3, 4, 1]), "RAS"],
        [np.diag([-2, 3, -4, 1])[:, [1, 0, 2, 3]], "ALI"],
        [np.diag([2, -3, 4, 1])[:, [0, 2, 1, 3]], "RSP"],
        [np.diag([-2, -3, -4, 1])[:, [2, 0, 1, 3]], "ILP"],
    ],
)
def test_aff2axcodes(affine: AffineMatrix4x4, axcode: StrictOrientationType):
    """Test whether aff2axcodes works as expected."""
    actual = "".join(aff2axcodes(affine, ("lr", "pa", "is")))
    expected = axcode.lower()
    assert actual == expected, "aff2axcodes did not return the expected axcode!"

My second test is a bit more complex and involves other function that are part of FastSurfer and not nibabel...

@effigies
Copy link
Member

effigies commented Feb 3, 2026

Would you please add the test to the PR?

Note that I will have limited internet access this week. Please ping again next Tuesday if I haven't gotten back to this.

Also, I'd like @leoyala's input on this approach. And @matthew-brett if you have the time.

@dkuegler
Copy link
Author

dkuegler commented Feb 4, 2026

It might actually be that @leoyala 's approach in #1450 is a similar solution to the same problem.

I guess I need to check if I just ran into a problem, that was almost concurrently found and fixed by someone else :)

@dkuegler
Copy link
Author

dkuegler commented Feb 5, 2026

I can confirm this same issue has been fixed in #1450

@dkuegler dkuegler closed this Feb 5, 2026
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.

2 participants

Comments