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

Replace numpy-quaternion with quaternionic. #507

Closed
wants to merge 2 commits into from

Conversation

CSSFrancis
Copy link
Member

Description of the change

As per #506.

I'm running into some problems making this change becuase I'm not really sure what the improper setter functions works with the _data attribute.

The difference arrises from quaternionic forcing having 4 as the last shape and not automatically casting as the numpy-quaternion package does.

import quaternionic as quaternion2
import quaternion
import numpy as np

quaternion.as_float_array(np.ones((10,4))).shape 
# (10, 4, 4)

quaternion2.array(np.ones((10,4))).shape
# (10,4)

@hakonanes any advice for what might need to change as a result?

Progress of the PR

For reviewers

  • The PR title is short, concise, and will make sense 1 year later.
  • New functions are imported in corresponding __init__.py.
  • New features, API changes, and deprecations are mentioned in the unreleased
    section in CHANGELOG.rst.
  • Contributor(s) are listed correctly in __credits__ in orix/__init__.py and in
    .zenodo.json.

@CSSFrancis
Copy link
Member Author

Okay so we might have to adjust how we do things.

  1. I think that the data for a Quaternion should always be a quaternion class from quaternionic.
  2. The rotation class should be Quaternion + a boolean for proper/ improper. I think the right thing to do would be to make a numpy array class for this but.... Maybe we should just sperate these two arrays into data + improper.

@pc494
Copy link
Member

pc494 commented Aug 1, 2024

It looks like we might get a downstream fix now, which would be excellent.

moble/quaternion#229

@CSSFrancis
Copy link
Member Author

CSSFrancis commented Aug 1, 2024

@pc494 thank goodness:) that was quickly turning into a bit of a nightmare to sort out

@hakonanes
Copy link
Member

Sorry for answering late here...

The dependency on numpy-quaternion is NOT important. We only use it for quaternion-quaternion and quaternion-vector multiplication. Our "old" implementation used Numba, and as far as I can remember, it was only twice as slow.

If numpy-quaternion becomes a hassle, I'd suggest we make it an optional dependency: use it when available, but fall back to Numba if it's not. What do you think?

@hakonanes
Copy link
Member

And I should say, I find storing the rotation handedness (proper -> right-handed, improper -> left-handed; or, if you will, the rotation followed by an inversion) as a fifth value 0 or 1 in the Rotation._data array is rather elegant and makes the implementation easier.

@pc494
Copy link
Member

pc494 commented Sep 7, 2024

I think this is safe to close.

@CSSFrancis any objection?

@CSSFrancis CSSFrancis closed this Sep 7, 2024
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