pyxem / orix

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

Adding Jit-accelerated to/from functions to quaternion class #450

Closed argerlt closed 11 months ago

argerlt commented 1 year ago

Description of the change

This is NOT a finished Pull request, just starting a request so we can have a discussion.

Short version

I reorganized Quaternion class and made it's "to/from" functionality consistently call from "_conversions.py"

Long version

I started working on Issue #374 (Purpose for Neo-Eulerian Classes), but it's been 9 months since that issue was started, and since then Neo-Eulerian vectors have been used a lot more on the plotting side of things. As such, removing them is no longer a "quick and easy" code simplification, even if it did make sense.

However, the use of NeoEuler in Quaternion and all child classes of Quaternion was purely for to/from functionality, so I removed those and added jit-accelerated versions of those functions to _conversions.py

I also reorganized the functions of Quaternion into groups to make them easier to navigate.

I also removed the Quaternion.from_neo_euler command with the thought of later removing the NeoEuler class altogether, but I would like someone more familiar with the use of Rodriguez and AxAngle to weight in before I go down that path. For now, I just replaced all the Rotation.from_neo_euler(Axangle.from_axis_angle(axis,angle)' calls with the much simplerRotation.from_axis_angle(axis,angle)`

Next step would be to either continue removing NeoEuler, or to re-add Quaternion.from_neo_euler and it's test functions, and leave the rest alone for now.

Progress of the PR

Minimal example of the bug fix or new feature

>>> from orix import vector
>>> v = vector.Vector3d([1, 1, 1])
>>> # Your new feature...

For reviewers

argerlt commented 1 year ago

Okay, I believe this Pull request is now done, barring any further reviewer feedback

There are a handful of things that could still be done

hakonanes commented 1 year ago

Thanks for again accomodating my review comments and suggestions, @argerlt. I agree that the PR is in good shape and that functionality is tested and documented. However, there are some style inconsistencies in both syntax and docstrings. Are you OK with me pushing some commits to your branch fixing these?

argerlt commented 1 year ago

Sure thing, go ahead.

hakonanes commented 1 year ago

I suggest we remove the Quaternion.from_rodrigues_frank() method and change Quaternion.from_rodrigues() either of these two ways:

  1. Allow an optional omega parameter for scaling the axis (combined being the Rodrigues-Frank vector). If users have their 4-component vectors in one array, they have to split it to pass it in. Only a minor hassle, I'd say.
  2. Allow ro to have either 3 (vector) or 4 (vector and scaling) as the final array dimension. Drawback could be that users mistake where the scaling should be (first or last).

@argerlt, what do you think?

I'm still going through the PR and making small adjustments here and there. I especially appreciate that you had a look at 3Drotations and have attempted to harmonize orix with output from this package.

argerlt commented 1 year ago

I went back and forth on this issue a lot, and I do think you are right, creating `Quaternion.from_rodrigues_frank()' was the wrong solution.

I like option one. It causes users inputting a 4-vector to make a conscious decision one way or another, which will probably help avoid errors, and its a method by which users who only know of the 3-vector version won't become unnecessarily confused.

I think option two might end up being a headache to implement, as there will be inevitable edge cases from accepting either a length 3 or 4 vector.

That said, I think both would work fine.

hakonanes commented 1 year ago

It seems to me like the fourth component in the Rodrigues-Frank vector expected in Quaternion.from_rodrigues_frank(ro) (to be removed) is not the angle omega but the scaling factor tan(omega / 2). Is this correct?

hakonanes commented 1 year ago

Yes, I see that you interpret omega as being the scaling factor in the docstring of that method. Good.

hakonanes commented 11 months ago

@argerlt, if it's fine by you, I'll rebase on develop and fix the merge conflict. The docs should then pass.

argerlt commented 11 months ago

Works for me. Thanks again, and sorry i let this sit so long.

hakonanes commented 11 months ago

No problem! I've been off development myself lately.

argerlt commented 11 months ago

@hakonanes , code has been rebased and everything seems to be working as expected. let me know if there are any other changes you would like to see.