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.

Sign up for GitHub

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

Jump to bottom

ENH: New Rotation method approx_equal() #17460

Merged
merged 4 commits into from Jul 28, 2023
Merged

ENH: New Rotation method approx_equal() #17460

merged 4 commits into from Jul 28, 2023

Conversation

scottshambaugh
Copy link
Contributor

@scottshambaugh scottshambaugh commented Nov 22, 2022 •
edited

Reference issue

Closes #17458

What does this implement/fix?

This implements two new methods to scipy.spatial.transform.Rotation objects.
This first is dist, which calculates the smallest angular distance between two rotations. For two rotations p and q, the following equivalence holds:

r = p * q.inv()
p.dist(q) == r.magnitude()

The second is approx_equal, which calculates the distance between rotations and checks if the angle is under an absolute tolerance of atol radians. The default atol = 1e-12 is notional and I'm all ears if there's a better number there.

r = p * q.inv()
p.approx_equal(q, atol) == (r.magnitude() < atol)

Additional information

@scottshambaugh scottshambaugh force-pushed the rotation_equality branch 2 times, most recently from 1902c92 to f9142b2 Compare November 22, 2022 06:34
@j-bowhay j-bowhay added enhancement A new feature or improvement scipy.spatial labels Nov 22, 2022
@scottshambaugh scottshambaugh force-pushed the rotation_equality branch 3 times, most recently from bd2d6e2 to ad15a6e Compare November 23, 2022 05:30
@scottshambaugh scottshambaugh force-pushed the rotation_equality branch 3 times, most recently from 67ed937 to 5d28ce4 Compare December 3, 2022 23:47
@scottshambaugh scottshambaugh marked this pull request as ready for review December 4, 2022 00:03
@scottshambaugh scottshambaugh mentioned this pull request Dec 4, 2022
Merged
@scottshambaugh
Copy link
Contributor Author

This is good to go.

@scottshambaugh scottshambaugh changed the title ENH: Distance between rotations and rotation equality checks ENH: New Rotation methods: rotation_to(), angle_to(), approx_equal() Dec 20, 2022
@scottshambaugh scottshambaugh force-pushed the rotation_equality branch 3 times, most recently from 8839630 to bb86868 Compare December 23, 2022 15:52
@scottshambaugh scottshambaugh force-pushed the rotation_equality branch 3 times, most recently from 1eddc6b to 212ceeb Compare January 8, 2023 23:59
@scottshambaugh scottshambaugh force-pushed the rotation_equality branch 3 times, most recently from 188dd59 to 44c06c1 Compare February 5, 2023 00:17
@scottshambaugh
Copy link
Contributor Author

Sounds good, updated that tolerance to 1e-8.

@h-vetinari h-vetinari mentioned this pull request May 24, 2023
Merged
@tylerjereddy
Copy link
Contributor

I'll bump the milestone since I need to branch soon. If a core developer is confident enough to merge before I branch, they're free to do so and restore the milestone, but this isn't a blocker for branching.

@tylerjereddy tylerjereddy modified the milestones: 1.11.0, 1.12.0 May 29, 2023
@nmayorov
Copy link
Contributor

My opinion is that only appxox_equal should go in.

approx_equal is indeed the only correct way to compare rotations and not completely trivial.

There is another possible implementation of rotation_to as p p.rotation_to_2(q) = q. So this "intuitive" method is ambiguous, I don't think it's necessary in the library API.

@scottshambaugh
Copy link
Contributor Author

Thank you for the review @nmayorov! I agree that appxox_equal is the really important function to add - the others were just because. I took those out now.

@scottshambaugh scottshambaugh changed the title ENH: New Rotation methods: rotation_to(), angle_to(), approx_equal() ENH: New Rotation method approx_equal() Jul 28, 2023
nmayorov
nmayorov reviewed Jul 28, 2023
View reviewed changes
scipy/spatial/transform/_rotation.pyx Outdated
@cython.boundscheck(False)
@cython.wraparound(False)
cdef inline void _compose_quat_single( # calculate p * q into r
cdef inline double _quat_magnitude(const double[:] q):
Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly, I would prefer not to have these additional changes in this PR.

If you think it's important to refactor usage of magnitude computations, you can easily do it in other PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, removed them and addressed your other comments.

nmayorov
nmayorov approved these changes Jul 28, 2023
View reviewed changes
Copy link
Contributor

@nmayorov nmayorov left a comment

Choose a reason for hiding this comment

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

Nothing to complaint about, except minor docstring formatting.

scipy/spatial/transform/_rotation.pyx Outdated Show resolved Hide resolved
scipy/spatial/transform/_rotation.pyx Show resolved Hide resolved
scipy/spatial/transform/_rotation.pyx Show resolved Hide resolved
scipy/spatial/transform/tests/test_rotation.py Outdated Show resolved Hide resolved
@scottshambaugh scottshambaugh force-pushed the rotation_equality branch 3 times, most recently from ec244f2 to 0c93346 Compare July 28, 2023 16:01
@nmayorov
Copy link
Contributor

Looks perfect. As there was not much discussion I think I can merge it now.

@nmayorov nmayorov merged commit f42ef0b into scipy:main Jul 28, 2023
25 checks passed
@h-vetinari
Copy link
Member

Please remember to update the release notes for all the Rotation PRs. :)

@nmayorov
Copy link
Contributor

@h-vetinari done, thanks for reminding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rkern rkern rkern left review comments

@mgeier mgeier mgeier left review comments

@h-vetinari h-vetinari h-vetinari left review comments

@nmayorov nmayorov nmayorov approved these changes

@larsoner larsoner Awaiting requested review from larsoner

@rgommers rgommers Awaiting requested review from rgommers

@perimosocordiae perimosocordiae Awaiting requested review from perimosocordiae

@ilayn ilayn Awaiting requested review from ilayn

@andyfaff andyfaff Awaiting requested review from andyfaff

@steppi steppi Awaiting requested review from steppi

Assignees
No one assigned
Labels
enhancement A new feature or improvement scipy.spatial
Projects
None yet
Milestone
1.12.0
Development

Successfully merging this pull request may close these issues.

ENH: Rotation equality checks
7 participants
@scottshambaugh @rkern @tylerjereddy @nmayorov @h-vetinari @mgeier @j-bowhay