Skip to content
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

question about rotation matrix ordering #9

Open
McHaillet opened this issue Nov 15, 2024 · 4 comments
Open

question about rotation matrix ordering #9

McHaillet opened this issue Nov 15, 2024 · 4 comments

Comments

@McHaillet
Copy link
Member

Hey, I was looking at the code to add some 2D->1D slices extraction, but was wondering about this line

# rotation matrices rotate xyz coordinates, make them rotate zyx coordinates

I understand from this that the input matrices are expected to work on xyz coordinates. Could we perhaps add a keyword to the function for zyx matrices?

@alisterburt
Copy link
Member

good spot - I wasn't sure what the best API was here so would appreciate some input/discussion

Previously (in libtilt) I seem to remember having a zyx: bool keyword argument. The options I considered for this package were

  • having a zyx keyword argument
  • only accepting zyx matrices
  • only accepting xyz matrices

I felt like the presence of the keyword argument made reasoning about the function harder. I felt like only accepting zyx matrices was going to lead to a bunch of unexpected failures for people who don't have any idea what that means, it requires a deeper understanding about how the matrices are being used inside the function and numpy (C style) axis ordering/memory layout for arrays. xyz is inelegant in some ways but felt like the least bad choice... I'm very open to changing to either of the other options - my preference would be zyx only I think if we switched...

One reason I considered not switching was that the scipy Rotation object produces matrices which are xyz

@McHaillet
Copy link
Member Author

Difficult question, also as its mainly a matter of convention.

Previously (in libtilt) I seem to remember having a zyx: bool keyword argument.

Exactly, I got used to it by now from working with that and have them in zyx order in tttsa. Once all the tensors stick to dhw, it makes more sense to me that coordinates are also in zyx (as you say C style). So, now it would feel confusing again to call functions from here with that order.

One reason I considered not switching was that the scipy Rotation object produces matrices which are xyz

It is normally the convention, also Wikipedia documents it like that for example. ( btw the cryotypes ProjectionModel also produces xyz BTW.

To me it makes the most sense to stick to xyz by default (for standard convention reasons), but provide a keyword for zyx order. Someone without too much experience gets the right behaviour when putting in scipy Rotations, but than it can still be set.

@alisterburt
Copy link
Member

alright, so we agree that xyz default is sane, we agree that zyx is nice sometimes so the question is whether the complication of the API is worth it... given that you brought it up I'm gonna go with yes :-)

Let's change to having zyx as a keyword argument which defaults to false and is properly documented

@McHaillet
Copy link
Member Author

Sounds good to me!

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

No branches or pull requests

2 participants