pyxem
Add class method 'from_align_vectors()' to Quaternion, Misorientation and Orientation
Description of the change
This PR is a result of an ongoing recent discussion.
I have made functions to wrap scipy.spacial.transform.Rotation.align_vectors() to initialize Quaternion, Misorientation or Orientation objects which rotates a set of vectors b into another set a. For the Misorientation and Orientation objects, symmetry arguments are also accepted.
For the Quaternion class, I have also made a function which initializes a Quaternion object from a Scipy Rotation object.
The implementation of multiple return values in my code is not very elegant, and suggestions for making it better is appreciated.
In the discussion, the conversion from Scipy quaternions to Orix quaternions is discussed. I did look a little into this, and got very confused, but the current conversion in Quaternion.from_scipy_rotation() seems to work in the sense that the resulting orientation from the function Quaternion.from_align_vectors() indeed transforms vectors from b to a.
Progress of the PR
- [X (ish) ] Docstrings for all functions
- [ ] Unit tests with pytest for all lines
- [X] Clean code style by running black via pre-commit
Minimal example of the bug fix or new feature
>>> from orix.quaternion import Orientation
>>> from orix.vector import Vector3d, Miller
>>>
>>> crystal_millers = Miller([[2,-1,0],[0,0,1]])
>>> sample_vectors = Vector3d([[3,1,0],[-1,3,0]])
>>>
>>> ori = Orientation.from_align_vectors(crystal_millers, sample_vectors)
>>> print(crystal_millers)
>>> print(ori * sample_vectors.unit * crystal_millers.length)
Output:
Miller (2,), point group None, xyz
[[ 2 -1 0]
[ 0 0 1]]
Vector3d (2,)
[[ 2. -1. -0.]
[-0. -0. 1.]]
For reviewers
- [x] The PR title is short, concise, and will make sense 1 year later.
- [n/a] 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__inorix/__init__.pyand in.zenodo.json.
Thank you for the excellent feedback, @hakonanes. Glad to see the test you wrote works:)
Regarding the sorting of the parameters, I have inserted the symmetry argument in method from_align_vectors() for Orientation and Misorientation before the weights argument. The reason for this is that I, and I suspect most people, will use the symmetry argument much more than the weights argument.
@anderscmathisen, feel free to question my comments.
From https://github.com/pyxem/orix/discussions/400#discussioncomment-3826033:
This would be a super helpful function for me as well, as parent/child orientation relationships are often similarly given as vectors to avoid confusion, but MDF and ODF calculations require that same info as misorientations.
@argerlt, does the example I included above (https://github.com/pyxem/orix/pull/401#pullrequestreview-1139428658) look like what you're after?
@anderscmathisen, feel free to question my comments.
From #400 (comment):
This would be a super helpful function for me as well, as parent/child orientation relationships are often similarly given as vectors to avoid confusion, but MDF and ODF calculations require that same info as misorientations.
@argerlt, does the example I included above (#401 (review)) look like what you're after?
I believe so. I was actually coming here to give my suggestion for a from_scipy function, but it looks like the method @anderscmathisen worte passes your tests, so I'm happy!
@anderscmathisen, feel free to question my comments.
From #400 (comment):
This would be a super helpful function for me as well, as parent/child orientation relationships are often similarly given as vectors to avoid confusion, but MDF and ODF calculations require that same info as misorientations.
@argerlt, does the example I included above (#401 (review)) look like what you're after?
Yes it does. I was actually getting ready to submit a PR with my own from_scipy function, but @anderscmathisen 's version is is shorter and still passes all the suggested tests, so I have no feedback.
I have added some examples in the docstrings of the methods. These are probably not the best examples, but probably work fine? I would however have liked to have a better method of displaying the results rather than np.allclose() and return value True, but the docstring examples test code gave me errors when I used print. Any ideas?
The test I added for Quaternion.from_scipy_rotation method fails for some reason, even though I think the code should be ok. This is probably related to what @argerlt wrote in the discussion, but Im not sure.
The other unit tests are probably a little too focused on checking the type of the return values, idk, these are my first tests. Feel free to give feedback. Perhaps it would have been better to just use the tests you have written in this PR, @hakonanes ?
Perhaps it would have been better to just use the tests you have written in this PR, @hakonanes ?
I think your tests are fine. I'd like to add my examples myself as examples in the doc examples section after this PR is ready, if that's OK with you.
How would you like your name listed in the credits @anderscmathisen, "Anders Christian Mathisen"? I'll add your name with NTNU as the affiliation in the Zenodo metadata file.
And this is the changelog entries I plan to add to your branch
- Creation of ``Quaternion``(s) (or instances of inheriting classes) from SciPy
``Rotation``(s).
- Creation of one ``Quaternion`` or ``Rotation`` by aligning sets of vectors in two
reference frames, one ``Orientation`` by aligning sets of sample vectors and crystal
vectors, and one ``Misorientation`` by aligning two sets of crystal vectors in two
differente crystals.
Thanks for the ping @hakonanes, I will try to have a look a this before tomorrow!
How would you like your name listed in the credits @anderscmathisen, "Anders Christian Mathisen"? I'll add your name with NTNU as the affiliation in the Zenodo metadata file.
Anders Christian Mathisen is fine 👍🏼 The changelog also looks good to me.
I'm happy with this PR now, great work @anderscmathisen!
Before merging, I'd like to wait for @harripj or @argerlt to give their take on my question at the bottom of #401 (comment).
I left a comment describing my approach to solving the testing problem. If you are interested and this issue is still open October 25th, I can write up a from_scipy function that uses rotations as the pass-between method, but I am swamped until then. Otherwise, @anderscmathisen or @hakonanes , feel free to write your own version based on my suggestion, or ignore my comment entirely in favor of a different approach.
If you DO want to write up scipy2orix via rot method yourself, I believe the method should be something like this:
@classmethod
def from_scipy_rotation(cls,assume_active_input=True):
if assume_active_input == True:
passive_matrix = np.linalg.inv(scipy_quat.as_matrix())
else:
passive_matrix =scipy_quat.as_matrix()
orix_quat = Orientation.from_matrix(passive_matrix)
And the check would be something like:
# check bunge conversion
passive_bunges = (np.random.rand(1000,3) - 0.5)*np.array([np.pi*2,np.pi,np.pi*2])
active_bunges = passive_bunges[...,::-1]*(-1)
scipy_rot = R.from_euler('ZXZ',active_bunges).as_matrix()
orix_rot = Orientation.from_euler(passive_bunges).to_matrix()
assert(np.max(scipy_rot-orix_rot)<1E-10
but obviously much better formatted. Hope this helps!
Sorry for the delay all, and thanks for the interesting discussion! My only comment regarding inverting or not the Scipy rotation is that we document it in Notes of the docstring of from_scipy_rotation() so that anyone interested can see the interpretation (and then adjust for their use case as required).
As for deciding a default interpretation as discussed, given that SciPy rotations are active, I also think we should invert internally so that a SciPy rotation will be consistent in the orix framework.
I think my last commits here are consistent with our discussions on this PR. The current implementation uses matrix for scipy conversion and also inverts the scipy rotation. I added a short note in the docstring of from_scipy_rotation about the inversion. It's quite short, and perhaps not very easy to understand for new users?