pyxem / orix

Analysing crystal orientations and symmetry in Python
https://orix.readthedocs.io
GNU General Public License v3.0
79 stars 45 forks source link

Moved to and from class methods from Rotation to Quaternion #406

Closed anderscmathisen closed 1 year ago

anderscmathisen commented 1 year ago

Description of the change

Because of discussions in #401, #345 and #373, it seemed like there is a need to move the to_ and from_ methods of Rotation to the Quaternion class. And so I have copy pasted them over.

I have changed little to nothing about the methods themselves, other than changing names of variables and updated docstrings.

I did not move the random_vonmises() method as that would also require the angle_with method of Rotation class to be moved as well, and I was unsure whether that should be done or not.

I included an override of from_euler in Rotation to keep the proper vs improper rotation notion as it was originally.

Progress of the PR

Minimal example of the bug fix or new feature

>>> from orix.quaternion import Quaternion, Rotation
>>> import numpy as np
>>> euler = np.random.rand(10, 3)
>>> rot = Rotation.from_euler(euler)
>>> quat = Quaternion.from_euler(euler)

For reviewers

hakonanes commented 1 year ago

We haven't considered that quaternions in orix are not by default unit quaternions, however rotations are unit quaternions. We ensure this by normalizing rotations upon initialization, while quaternions are not normalized.

The conversions we have implemented are based on the work by Rowenhorst et al. (2015) https://doi.org/10.1088/0965-0393/23/8/083501, where they in the abstract state that the transformations in that work including quaternions concern unit quaternions. Hence, I suggest that all new

  1. from_x() methods return unit quaternions
  2. to_x() methods normalize before returning x

I've already done this in my fork of your branch here, @anderscmathisen, among other changes like moving some rotation tests upstream to the quaternion test file. I suggest I make a PR to your branch, merge that, and then someone else has to review this PR since it includes my work...

anderscmathisen commented 1 year ago

Yeah, I agree with your suggestions and nice that you have implemented them in a branch. Please let me know if I need to do anything.

hakonanes commented 1 year ago

I can fix the conflicts.

hakonanes commented 1 year ago

Don't know if you've fixed lots of git conflicts @anderscmathisen, but what I did was basically (on the move_to_from_methods branch):

> git pull anderscmathisen move_to_from_methods  # Update my local fork of your branch
> git merge develop
Auto-merging CHANGELOG.rst
CONFLICT (content): Merge conflict in CHANGELOG.rst
Auto-merging orix/quaternion/quaternion.py
Auto-merging orix/quaternion/rotation.py
CONFLICT (content): Merge conflict in orix/quaternion/rotation.py
Auto-merging orix/tests/quaternion/test_rotation.py
CONFLICT (content): Merge conflict in orix/tests/quaternion/test_rotation.py
Automatic merge failed; fix conflicts and then commit the result.

Open the conflicting files and do the necessary changes, in this case remove the changes on develop and keep the ones on move_to_from_methods, then:

> git add .
> git commit -s
> git push anderscmathisen move_to_from_methods  # Update your branch
hakonanes commented 1 year ago

I can add a small test covering the single line missing (https://coveralls.io/builds/53787279/source?filename=orix%2Fquaternion%2Frotation.py#L581).

hakonanes commented 1 year ago

To ease reviewer(s) minds...:

hakonanes commented 1 year ago

Fixed another conflict.

hakonanes commented 1 year ago

Great, thanks @pc494! We can now rebase #401 on this and get that PR done as well.