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

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

Merged
merged 15 commits into from
Nov 30, 2022

Conversation

anderscmathisen
Copy link
Contributor

@anderscmathisen anderscmathisen commented Oct 21, 2022

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

  • 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.

Copy link
Member

@hakonanes hakonanes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for setting this up @anderscmathisen!

I've made just a few suggestions for improvements, mostly to docstring wordings I or other people originally added...

orix/quaternion/quaternion.py Outdated Show resolved Hide resolved
orix/quaternion/quaternion.py Outdated Show resolved Hide resolved
orix/quaternion/quaternion.py Outdated Show resolved Hide resolved
orix/quaternion/quaternion.py Outdated Show resolved Hide resolved
orix/quaternion/quaternion.py Outdated Show resolved Hide resolved
orix/quaternion/rotation.py Outdated Show resolved Hide resolved
orix/quaternion/rotation.py Outdated Show resolved Hide resolved
orix/quaternion/rotation.py Outdated Show resolved Hide resolved
orix/quaternion/rotation.py Outdated Show resolved Hide resolved
Copy link
Member

@hakonanes hakonanes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. As I can see it's just one check in Quaternion.identity() missing, and we can merge this!

orix/quaternion/quaternion.py Outdated Show resolved Hide resolved
@hakonanes
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

hakonanes commented Nov 1, 2022

I can fix the conflicts.

@hakonanes
Copy link
Member

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
Copy link
Member

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

@hakonanes
Copy link
Member

To ease reviewer(s) minds...:

  • Both @anderscmathisen and I have contributed to this PR. He opened it, and then I pushed some changes due to Moved to and from class methods from Rotation to Quaternion #406 (comment).
  • Most of the changes are moving methods from Rotation to Quaternion, with most of the corresponding tests also moved between test files
  • Many other changes are just due to me sorting elements in the two classes: attributes, dunder (double underscore), classmethods, public methods, private methods. I find it easier to navigate a class with some structure. This becomes more important as new functionality is added.
  • I've looked through the user guide (tutorials and examples) in the docs a couple times and haven't seen any unwanted effects of the present changes.
  • Methods are kept in Rotation to (1) maintain a docstring refering to "rotations" and not "quaternions" and (2) list them under Rotation methods in the API docs

@hakonanes
Copy link
Member

Fixed another conflict.

@hakonanes hakonanes requested a review from pc494 November 4, 2022 18:26
Copy link
Member

@pc494 pc494 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay all, I'm happy with this. Because it's been a little while since the request, I'm going to let @hakonanes merge just in case something else has happened since that I'm not aware of.

@hakonanes hakonanes merged commit 1dec46a into pyxem:develop Nov 30, 2022
@hakonanes
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants