Fix 0º and 180º singularity cases in _rotation_matrix_from_vectors #4206
Conversation
WalkthroughThe update introduces a method to handle the calculation of skew-symmetric matrices for 3D vectors and refines the rotation matrix computation to address a specific edge case. This edge case arises when the angle between two vectors is exactly 180º, which previously led to a Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- fiftyone/utils/utils3d.py (2 hunks)
- tests/unittests/utils/utils3d_tests.py (1 hunks)
Additional comments: 3
tests/unittests/utils/utils3d_tests.py (1)
- 13-34: The test function
test_rotation_matrix_from_vectorsis well-structured and covers a comprehensive range of scenarios, including the critical singular cases of 0º and 180º rotations. It's good practice to add a brief comment explaining the choice ofatol=1e-15in thenp.testing.assert_allclosecalls, as this helps maintainers understand the tolerance level for floating-point comparisons.fiftyone/utils/utils3d.py (2)
- 778-782: The implementation of
_skew_symmetric_matrixis correct and follows the mathematical definition of a skew-symmetric matrix for a 3D vector. Well done.- 785-809: The modifications to
_rotation_matrix_from_vectorsto handle the 180º rotation singular case by selecting a random perpendicular vector are logically sound. However, consider enhancing the comment on line 800 to explain why this approach is necessary and how it resolves the issue with singular cases. Additionally, ensure that the function always returns a valid rotation matrix, especially in edge cases wherevec1andvec2might be nearly parallel or antiparallel.
|
Regarding this.
It's better to handle it an a separate PR because it requires touching a public facing API and it needs discussion on what to do with current way of specifying the camera orientation. |
Def agree! Code Rabbit keeping us honest |
fiftyone/utils/utils3d.py
Outdated
|
|
||
| if abs(c + 1) > np.finfo(c.dtype).eps: | ||
| K = _skew_symmetric_matrix(v) | ||
| return np.eye(3) + K + K.dot(K) / (1 + c) |
There was a problem hiding this comment.
Nice, I had seen this simplification also
There was a problem hiding this comment.
I use this Rotation module a lot myself in #3985
There was a problem hiding this comment.
If scipy is a valid dependency for this project (didn't even consider) than there's no point in having _rotation_matrix_from_vectors at all. We should just use Rotation.align_vectors. Less code to maintain and this PR can be reduced to a 1-2 LoC diff.
Edit: A bit more. scipy rightfully complains, so this needs to be suppressed.
UserWarning: Optimal rotation is not uniquely or poorly defined for the given sets of vectors.
Kabsch / Wabba are usually used to find the optimal rotation between point clouds. For a fully defined rotation one needs at least 3 non-collinear points.
There was a problem hiding this comment.
scipy was added as a dependency very very recently, and is not in any release yet. But it's already been implicitly required for a while, as we require scikit-learn and likely others that require scipy.
I would definitely be a big fan of using a module of its stature rather than rolling our own.
Please make this change, and if there are any other functions/operations that would more appropriately be replaced by scipy, let us know or add to this PR!
| """Validates the returned rotation matrix transforms rotates vec1 to vec2.""" | ||
| R = fou3d._rotation_matrix_from_vectors(vec1, vec2) | ||
|
|
||
| vec1_rot = R @ vec1 |
There was a problem hiding this comment.
whoa I never knew this operator existed ... (named as def __matmul__)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4206 +/- ##
===========================================
+ Coverage 28.85% 28.87% +0.01%
===========================================
Files 768 769 +1
Lines 97048 97151 +103
Branches 1120 1128 +8
===========================================
+ Hits 28004 28050 +46
- Misses 69044 69101 +57
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- fiftyone/utils/utils3d.py (1 hunks)
- tests/unittests/utils3d_tests.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- fiftyone/utils/utils3d.py
Additional comments: 1
tests/unittests/utils3d_tests.py (1)
- 284-307: The test method
test_rotation_matrix_from_vectorsis well-structured and covers essential cases, including the critical singular cases of 0º and 180º rotation. It's important to ensure direct testing of the_rotation_matrix_from_vectorsmethod to accurately assess its behavior in these edge cases. Consider adding a comment to clarify that this method is being directly tested, which is crucial for understanding the context of these tests.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- fiftyone/utils/utils3d.py (2 hunks)
- tests/unittests/utils3d_tests.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/unittests/utils3d_tests.py
There was a problem hiding this comment.
Sorry @SergioRAgostinho I forgot to circle back on this. LGTM, thanks for added tests 👍🏼
Congrats on your first contribution, welcome to the FiftyOne family!! Looking forward to more contribs from you 😃
What changes are proposed in this pull request?
Fixes #4203.
The method _rotation_matrix_from_vectors is ill-defined and returnes
NaNwhen the angle betweenvec1andvec2was 0º or 180º.From a geometrical standpoint the issue is being cause by the fact that there is no longer a single "shortest" rotation that transforms vec1 to vec2 There are now infinite perpendicular axes to vec1 and vec2 that can be used to rotate vec1 onto vec2.
This PR addresses that.
How is this patch tested? If it is not, please explain why.
With unit tests validating exactly the cases described above, as well as the original cases.
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
sp.transform.Rotation.align_vectors.