Fix non-deterministic axis for 180-deg rotations (#366) - #372
Open
TabishShahMohsin wants to merge 2 commits into
Open
Fix non-deterministic axis for 180-deg rotations (#366)#372TabishShahMohsin wants to merge 2 commits into
TabishShahMohsin wants to merge 2 commits into
Conversation
Ensure norm_axis_angle and norm_axis_angles always return a deterministic rotation axis for angles of exactly pi by flipping negative leading non-zero vector components. Adds unit tests.
There was a problem hiding this comment.
Pull request overview
This PR addresses Issue #366 by making norm_axis_angle and norm_axis_angles return a deterministic axis for 180° (π rad) rotations, removing the sign ambiguity between axis and -axis at π. It also adds unit tests to validate the deterministic behavior in both scalar and batch APIs.
Changes:
- Canonicalize the rotation axis sign at angle π so the first non-zero component is positive.
- Add scalar and batch unit tests covering negative-axis inputs at π.
- Update normalization behavior documentation/tests for the deterministic π-axis rule.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
pytransform3d/rotations/_axis_angle.py |
Adds deterministic axis sign normalization for π rotations in norm_axis_angle. |
pytransform3d/batch_rotations/_axis_angle.py |
Adds deterministic axis sign normalization for π rotations in norm_axis_angles. |
pytransform3d/rotations/test/test_axis_angle.py |
Adds unit test for deterministic axis output at π in scalar normalization. |
pytransform3d/batch_rotations/test/test_batch_rotations.py |
Adds unit test for deterministic axis output at π in batch normalization. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+97
to
+107
| # Issue #366: Make axis deterministic for 180 degree rotations | ||
| # the first non-zero component of axis should be positive. | ||
| if np.isclose(angle, np.pi): | ||
| is_zero = np.isclose(res[:3], 0.0) | ||
|
|
||
| non_zero_indices = np.where(~is_zero)[0] | ||
|
|
||
| if len(non_zero_indices) > 0: | ||
| first_non_zero_val = res[non_zero_indices[0]] | ||
| if first_non_zero_val < 0.0: | ||
| res[:3] *= -1.0 |
Comment on lines
+97
to
+99
| # Issue #366: Make axis deterministic for 180 degree rotations | ||
| # the first non-zero component of axis should be positive. | ||
| if np.isclose(angle, np.pi): |
|
|
||
| # Issue #366: Make axis deterministic for 180 degree rotations | ||
| # the first non-zero component of axis should be positive. | ||
| pi_mask = np.isclose(res[..., 3], np.pi) & rot_mask |
Comment on lines
+289
to
+298
| def test_norm_axis_angle_180_degrees_deterministic(): | ||
| a1 = np.array([-1.0, 0.0, 0.0, np.pi]) | ||
| res1 = pr.norm_axis_angle(a1) | ||
| np.testing.assert_array_almost_equal(res1[:3], [1.0, 0.0, 0.0]) | ||
| a2 = np.array([0.0, -1.0, 0.0, np.pi]) | ||
| res2 = pr.norm_axis_angle(a2) | ||
| np.testing.assert_array_almost_equal(res2[:3], [0.0, 1.0, 0.0]) | ||
| a3 = np.array([0.0, 0.0, -1.0, np.pi]) | ||
| res3 = pr.norm_axis_angle(a3) | ||
| np.testing.assert_array_almost_equal(res3[:3], [0.0, 0.0, 1.0]) |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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.
Ensure norm_axis_angle and norm_axis_angles always return a deterministic rotation axis for angles of exactly pi by flipping negative leading non-zero vector components. Adds unit tests.
Fixes #366