Skip to content

Conversation

@thewtex
Copy link
Member

@thewtex thewtex commented Jul 2, 2021

No description provided.

@thewtex
Copy link
Member Author

thewtex commented Jul 2, 2021

@andy-sweet having some difficulty getting the signs consistently out of scale and rotate.

As noted in napari/napari#2163 (comment)

One other thing to watch out for is that napari currently absorbs reflections into the scale component (i.e. the scale component can contain negative values), so the scale, translate, and rotate parameters may not always come out exactly as they went in (i.e. if the rotate matrix passed in has a determinant of -1, the one returned will have a determinant of 1 and corresponding scale parameter will be negated. I think we can change that without too much work though.

But I am not seeing a consistent pattern in the proposed test values or how to handle them. Any suggestions?

@andy-sweet
Copy link

Sorry for the slow response Matt. I pulled down this branch and also found that test_image_layer_from_image_rotate fails when checking the non-zero angles.

I think there's at least two things going on here.

  1. ITK's ImageBase::SetDirection seems to modify the matrix passed in. You can check this with assert np.allclose(image['direction'], rotate), which fails for me for angle=30. I think this is just a transpose, as passing into rotate.T fixes this issue for me, but it would be good to confirm.

  2. After using rotate.T as described in (1), the tests pass until angle=120 when they fail again. It looks like the code we're using to decompose our affine matrix isn't quite doing what we want. In this case, image_layer.scale == [-1, -1] and so image_layer.rotate actually corresponds to a rotation of 300 (or -60) degrees because of the double reflection (from the original 120 degrees).

We can probably fix our decomposition code to fix (2) by checking when there are an even number of reflections and conforming scale and rotate appropriately (if I'm remembering my geometry/linear algebra correctly), but this bug makes me more inclined to not depend on that decomposition and just store the transformation components separately.

Either way, I'll file a bug on Monday and will try to fix this by the end of next week to unblock you.

@andy-sweet
Copy link

Point (2) should be fixed now that napari's PR #2990 has been merged, after which we avoid the decomposition when it's not necessary and just store the transform components as they were passed in (or at least in some standardized form of that).

I think you may still need a transpose in the napari layer <-> ITK image conversion though.

Convert the itk.Image Direction to the napari Image layer `rotate` and
back.
@thewtex thewtex changed the title WIP: ENH: Add rotate support ENH: Add rotate support Oct 11, 2025
@thewtex
Copy link
Member Author

thewtex commented Oct 11, 2025

@andy-sweet thank you so much for the improvements and comments. After jumping in a time machine ⏲️ your improvements are now released in napari, and I made the suggested transpose change.

@N-Dekker please take a look 🙏

@thewtex thewtex merged commit 80e0ce2 into main Oct 16, 2025
13 checks passed
@thewtex thewtex deleted the rotate branch October 16, 2025 02:37
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.

3 participants