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

MAINT/DOC: spatial: Document and test the double cover property for Rotation quaternions #18955

Merged
merged 4 commits into from Jul 27, 2023

Conversation

scottshambaugh
Copy link
Contributor

@scottshambaugh scottshambaugh commented Jul 24, 2023

What does this implement/fix?

We represent Rotations under the hood using unit quaternions. Quaternions have an interesting property in that they cover the space of rotations twice (the SU(2) group), such that q and -q represent the same orientation of 3D space (the SO(3) group). This can be thought of as representing 720 degrees of rotation instead of 360 degrees. For most use cases this is little more than trivia, however in some instances preserving this double cover property can be useful. See here for an example.

In #17334 (released in v1.11.0), the inv() method was fixed to properly preserve the double cover behavior. This gives the Rotation class a minimally viable subset of methods to work in the double cover space. See also that issue for a lot more discussion about this behavior.

This PR adds documentation for this behavior, and some tests to ensure it does not regress in future updates.

I think it makes most sense to put this info in the from_quat docstring, but if the top level class docs is better I can move that over.

Additional information

The new reference is here, and appears to be open access. https://dl.acm.org/doi/book/10.5555/2821580
Hanson, Andrew J. "Visualizing quaternions." Morgan Kaufmann Publishers Inc., San Francisco, CA. 2006.

@j-bowhay j-bowhay added the Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org label Jul 24, 2023
``r`` corresponding to a quaternion ``q`` are guaranteed to preserve
the double cover property: ``r = Rotation.from_quat(q)``,
``r.as_quat(canonical=False)``, ``r.inv()``, and composition using the
``*`` operator such as ``r*r``.
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I read through some of the linked open access textbook and the content here is reasonably sensible to me, though I'm certainly not an expert.

Can you clarify for me/remind me what happens for those operations for which application of -q doesn't have the same effect as q? Is this just "-1 always maps to 1 for redundant cases for supported operations" kind of thing? Since double cover seems to be a fundamental property of quaternions are there "problems" with application of -q and q for those ops we have that don't support "double cover?"

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, maybe this is more innocent than I think? A bit like preserving IEEE signed zero semantics, where sure you can have -0. and +0, but really you'll get the same answer for the vast majority of ops so it is kind of a formality? In this case you're saying that there is functionality to prevent canonicalization to the single "positive" representation for the "supported" ops?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we're taking particular care to ensure that we are leaving the quaternions as "native" as we can in those operations and not making unnecessary use of the fact that q and -q represent the same 3D rotation. For example, to compute the inverse rotation in the quaternion representation, one negates the vector part of the quaternion (hypercomplex conjugation; conjugate(q)). Previously, we negated the real part instead (e.g. equivalent to conjugate(-q)) and relied on the fact that both of those quaternions represented the same rotation (possibly as a microptimization because only one column needed to get negated). Now, we are implementing Rotation.inv() as raw quaternion conjugation. For the most part, if you're not peeking at the quaternion representation, it won't matter, but if you are and doing some fancy things with the quaternion representations (e.g. various kinds of interpolation or dealing with multivalue Rotation objects as connected paths), then it can matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, agree with all the above. This should have no impact on any operations outside of those specifically using the quaternion double cover in the first place: having both -0 and +0 is a good analogy for the formality here.

scipy/spatial/transform/tests/test_rotation.py Outdated Show resolved Hide resolved
scipy/spatial/transform/tests/test_rotation.py Outdated Show resolved Hide resolved
@j-bowhay j-bowhay changed the title Document and test the double cover property for Rotation quaternions MAINT/DOC: spatial: Document and test the double cover property for Rotation quaternions Jul 26, 2023
@tylerjereddy tylerjereddy merged commit 897d811 into scipy:main Jul 27, 2023
22 of 26 checks passed
@tylerjereddy
Copy link
Contributor

Sounds like we're in agreement here and the CI fails were unrelated. I spot checked one or two of the test cases and seemed to make sense to me, and no old tests have changed behavior.

@tylerjereddy tylerjereddy added this to the 1.12.0 milestone Jul 27, 2023
scottshambaugh added a commit to scottshambaugh/scipy that referenced this pull request Jul 28, 2023
…otation quaternions (scipy#18955)

* Document and test the double cover property for quaternions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org scipy.spatial
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants