Add geodesic distance implementation stable around pi#28
Open
HampSwe wants to merge 2 commits into
Open
Conversation
Include the same tests for rotmat_geodesic_distance_pi_stable as rotmat_geodesic_distance. Note that this refactoring also tests the naive method for right invariance and the ordinary test; previously it was only tested for left invariance.
Contributor
|
Hi, thank you for the pull request (and sorry for not commenting earlier). Numerical precision close to π was indeed not one of my main concerns here. The relative error remains small with However, I would prefer not to add the additional Would you mind updating the pull request to remove it? |
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.
Both$\alpha=\pi$ , in the same way as $\alpha=0$ . This PR adds another implementation, $\alpha=\pi$ too (i.e. precise for all $\alpha$ ), at the price of slower runtime performance around $\alpha=\pi$ . See the figures below for the numerical improvement.
roma.utils.rotmat_geodesic_distanceandroma.utils.rotmat_geodesic_distance_naiveare numerically imprecise aroundroma.utils.rotmat_geodesic_distance_naiveis imprecise aroundroma.utils.rotmat_geodesic_distance_pi_stable, which is numerically precise atFor performance reasons,$\alpha$ is too close to $\pi$ given a tolerance, it recalculates the value of $\alpha$ using a more precise, but slower, function called $\alpha=atan2(sin(\alpha), cos(\alpha))$ , $sin(\alpha)=\frac{1}{2}|(R_{21}-R_{12}, R_{02}-R_{20}, R_{10}-R_{01})|_2$ and $cos(\alpha)=\frac{1}{2}(Trace(R)-1)$ . See https://en.wikipedia.org/wiki/Rotation_matrix#Axis_and_angle for more details on this implementation.
roma.utils.rotmat_geodesic_distance_pi_stablefirst callsroma.utils.rotmat_geodesic_distance. If the resultingroma.utils._rotmat_geodesic_distance_atan2.roma.utils._rotmat_geodesic_distance_atan2works by the formulasNote that$\alpha$ that are not close to $\pi$ ,
roma.utils._rotmat_geodesic_distance_atan2is significantly slower thanroma.utils._rotmat_geodesic_distance. In this implementation, which has been optimized for CUDA, it is approximately 3x slower on CUDA tensors and 10x slower on CPU tensors. One can change the implementation of it to reverse the results and instead achieve higher runtime performance on the CPU; a comment can be found in the implementation on how to do this. Of course, one could dispatch based on the given device, but for maintainability I have simply opted for the CUDA-friendly implementation. However, for allroma.utils.rotmat_geodesic_distance_pi_stableproduces the same result asroma.utils.rotmat_geodesic_distancewith only a minimal overhead cost in runtime performance.This PR also adds a specific test for this new implementation which
roma.utils.rotmat_geodesic_distancefails. It also updates the API docs, and makes a small comment under the "Care for numerical precision" section in the docs.