fix(space_ops): flip the axis when angle_axis_from_quaternion folds the angle - #4980
Open
dylanpulver wants to merge 1 commit into
Open
Conversation
…he angle A rotation by `angle` about `axis` equals a rotation by `TAU - angle` about `-axis`. The fold at the end of `angle_axis_from_quaternion` replaced the angle without negating the axis, so for any quaternion whose angle exceeds PI the returned pair described the inverse rotation. Adds a round-trip test that checks the returned pair against the input rotation matrix and against scipy's canonical `Rotation.as_rotvec` form.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview: What does this pull request change?
angle_axis_from_quaternion(manim/utils/space_ops.py:143) folds an angle abovePIwithbut leaves the axis untouched. A rotation by
angleaboutaxisis a rotation byTAU - angleabout
-axis, so for every quaternion whose angle exceedsPIthe returned pair describes theinverse rotation. This adds
axis = -axisto the fold, plus a test.Motivation and Explanation: Why and how do your changes improve the library?
On
main, feedingquaternion_from_angle_axis's own output back in does not round trip:5000 random (angle, axis) pairs, uniform angle over
[0, TAU): 2556 fail the round trip onmain, 0 after. The split is clean — all 2556 haveangle > PI, none haveangle <= PI. Theneighbouring
rotation_matrix_from_quaternionandrotation_matrix_transpose_from_quaternionwererun over the same 5000 inputs as a control: 0 failures each, before and after.
Which of the two possible remedies to take was settled by scipy, already a hard dependency and what
rotation_matrixin this same module builds on:Rotation.as_rotvecreturns exactly theangle in [0, PI]+ signed-axis form, and the patched function agrees with it on all 5000 cases.Simply deleting the fold also repairs the round trip, but changes the returned angle range — happy
to go that way instead if you prefer it. No test caught this because the function has zero call
sites inside
manim/and had no test.Matched pair, same command and env both times (
uv run pytest tests/module -q -p no:randomly):492 passed / 90 failed before → 499 passed / 90 failed after. The 90 are pre-existing here
because this machine has no LaTeX (
FileNotFoundError: 'latex'); they are identical on anunmodified checkout.
pre-commit run --files ...passes all hooks at the pinned versions(ruff 0.16.2 lint + format, mypy, codespell).
Two mutants, both with the source marker asserted before measuring:
PIfail, the 3 at or belowPIpass;[0, PI]assertion fails, which is what pins the remedy rather than just the round trip.
Links to added or changed documentation pages
angle_axis_from_quaternionin the space_ops reference; itsReturnstext now names the anglerange.
Further Information and Comments
Not tested: the LaTeX-dependent tests did not run here. I have not touched the
-> Sequence[float]return annotation, which does not describe the
(float, ndarray)tuple, to keep the diff minimal.Written with AI assistance (Claude); the measurements above were run locally on this branch.
Reviewer Checklist