Fix non-deterministic axis for 180-deg rotations (#366) by TabishShahMohsin · Pull Request #372 · dfki-ric/pytransform3d

TabishShahMohsin · 2026-08-14T11:07:04Z

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

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.

Copilot

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.

AlexanderFabisch · 2026-08-17T06:33:41Z

Hi @TabishShahMohsin , thanks for the contribution. I don't know why copilot decided to do the review, but I will start mine now. :)

AlexanderFabisch

Here are my comments.

AlexanderFabisch · 2026-08-17T07:26:56Z

+    if np.any(pi_mask):
+        axes = res[pi_mask, :3]
+        is_zero = np.isclose(axes, 0.0)
+        first_non_zero_idx = np.argmax(~is_zero, axis=1)


Can we rename this to first_non_zero_idx_per_entry or something similar?

What if the input is not a 2d array, but generally an N-dimensional array, e.g., an array of an array of axis-angle representations? axis=1 should be replaced by axis=-1.

What if the axis is [0, 0, 0]? This is an edge case.

I have corrected the axis=1 bug, and changed the variable names.

Thanks. Could you check [0, 0, 0, pi] explicitly? Does it return [1, 0, 0, 0] as suggested in the docstring?

AlexanderFabisch · 2026-08-17T07:29:00Z

+        axes = res[pi_mask, :3]
+        is_zero = np.isclose(axes, 0.0)
+        first_non_zero_idx = np.argmax(~is_zero, axis=1)
+        row_indices = np.arange(len(axes))


Why don't we just use : instead of np.arange?

Had to use this for (4, )

But does it work for, e.g., 3d arrays? Would first_non_zero_axis_components = axes_with_pi_rotation[..., first_non_zero_idx_per_entry] work? (Maybe not.) I just feel there should be a simpler solution than arange...

AlexanderFabisch · 2026-08-17T07:33:54Z

+    pi_mask = np.isclose(res[..., 3], np.pi) & rot_mask
+    if np.any(pi_mask):
+        axes = res[pi_mask, :3]
+        is_zero = np.isclose(axes, 0.0)


I am not entirely sure whether we want to use np.isclose here. At least the default threshold might not be appropriate. Which threshold does it use?

I noticed batch_rotations/_matrix.py line 51 (which comes from #43 ):
angle_close_to_pi = np.abs(angles - np.pi) < 1e-4
Also "tolerance=1e-6" is used at many many places, so I have added the same.

I think this matches too broadly. There is still valuable information in the sign of the rotation axis when the angle is within the range of 1e-4 or 1e-6 of pi. This might be critical in applications, in which high numerical precision is required.

1e-4 was specifically selected to bound the error of that conversion function. We would have to do a similar analysis for this case: measure the minimum of the angle between the normalized axes before and after normalization and abs(this value - pi) (or something similar). However, I think it is perfectly safe to just check angle == pi.

…ic_batch

…nt with others

AlexanderFabisch · 2026-08-27T09:44:40Z

+    for axis in axes:
+        a_random = np.hstack((axis, [np.pi]))
+        res_random = pbr.norm_axis_angles(a_random)
+        assert_array_almost_equal(res_random, pr.norm_axis_angle(a_random))


What if we flip the axis signs? Does it still produce the same axis consistently?

Please add this check.

AlexanderFabisch · 2026-08-27T09:45:49Z

+    # [0, 0, 0] 
+    a_zero_axis = np.array([0.0, 0.0, 0.0, np.pi])
+    res_zero_axis = pbr.norm_axis_angles(a_zero_axis)
+    assert_array_almost_equal(res_zero_axis, pr.norm_axis_angle(a_zero_axis))


The test does not make a lot of sense. Which axis-angle representation do we expect here exactly?

The docstring says [1, 0, 0, 0]. Is it the case here?

Then please write this explicitly. It is currently a circular test.

AlexanderFabisch · 2026-08-27T09:46:11Z

+    # (4,)
+    a_single = np.array([-1.0, 0.0, 0.0, np.pi])
+    res_single = pbr.norm_axis_angles(a_single)
+    assert_array_almost_equal(res_single, pr.norm_axis_angle(a_single))


Which result do we expect here?

[1.0, 0.0, 0.0, np.pi]

Then please state this explicitly in the test. It is currently a circular test.

AlexanderFabisch · 2026-08-27T09:51:58Z

    -------
    a : array, shape (4,)
        Axis of rotation and rotation angle: (x, y, z, angle). The length
        of the axis vector is 1 and the angle is in [0, pi). No rotation


Please correct the range to [0, pi] as we now return angles pi explicitly.

…as well for test_batch_rotations.py

TabishShahMohsin · 2026-09-02T10:52:58Z

Thanks for the thorough review and patience.

…eral values for non random tests

AlexanderFabisch · 2026-09-07T07:04:48Z


+    # Issue #366: Make axis deterministic for 180 degree rotations
+    # the first non-zero component of axis should be positive.
+    pi_mask = (np.abs(res[..., 3] - np.pi) < tolerance) & rot_mask


Checking res[..., 3] == np.pi seems to be the most reasonable solution. There seems to be no good argument for a broader range defined by threshold.

AlexanderFabisch · 2026-09-07T07:05:14Z


+    # Issue #366: Make axis deterministic for 180 degree rotations
+    # the first non-zero component of axis should be positive.
+    if np.abs(angle - np.pi) < tolerance:


Checking angle == np.pi seems to be the most reasonable solution. There seems to be no good argument for a broader range defined by threshold.

Exact floating comparison is fragile. It can break even on mathematically equivalent expressions.

pi = np.pi for i in range(1, 50): a = (pi / i) * i if a!=pi: print(i)

This prints 25 and 49.

I believe this is more a problem of the previous processing steps, not something that we have to or can fix here.

The problem is: When the computation is exact, accepting a broader range at this check messes up the result. It might result in a slightly wrong axis.

AlexanderFabisch · 2026-09-07T07:09:20Z

+        assert_array_almost_equal(res_random, pr.norm_axis_angle(a_random))
+        a_random = np.hstack((-axis, [np.pi]))
+        res_random = pbr.norm_axis_angles(a_random)
+        assert_array_almost_equal(res_random, pr.norm_axis_angle(a_random))


This test is still circular. I'd convert both the unnormalized and the normalized axis-angle representation to quaternion or rotation matrix and check whether they are identical.

I though that it so to make it consistent with the existing tests, have added the test.

…tion test

AlexanderFabisch · 2026-09-15T14:52:00Z

+            res_random = pbr.norm_axis_angles(a_random)
+
+            # scalar vs batch consistency check
+            assert_array_almost_equal(res_random, pr.norm_axis_angle(a_random))


This check still does not make any sense to me. Both the expected and the actual value go through exactly the same function.

Fix non-deterministic axis for 180-deg rotations (dfki-ric#366)

0a6aca9

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.

Copilot AI lite review requested due to automatic review settings August 14, 2026 11:07

Copilot started reviewing on behalf of TabishShahMohsin August 14, 2026 11:07 View session

Copilot AI reviewed Aug 14, 2026

View reviewed changes

AlexanderFabisch reviewed Aug 17, 2026

View reviewed changes

TabishShahMohsin added 3 commits August 26, 2026 00:44

Added new test cases for rotation.norm_axis_angle

60ff3b0

Remove .DS_Store from version control

6b9b5d1

Replaced np.isclose with tolerance in rotations/_axis_angle.py

cfb9a7a

TabishShahMohsin force-pushed the fix-180-deg-axis-deterministic branch from baa577c to cfb9a7a Compare August 25, 2026 19:40

TabishShahMohsin added 5 commits August 26, 2026 10:44

Added new test cases for test_norm_axis_angle_180_degrees_determinist…

011222e

…ic_batch

Corrected tests for test_norm_axis_angle_180_degrees_deterministic_batch

69b504e

Made the test test_norm_axis_angle_180_degrees_deterministic consiste…

0c038f0

…nt with others

Updated doc string for norm_axis_angle

a15d9b7

Updated doc string for norm_axis_angles

751e4bd

AlexanderFabisch reviewed Aug 27, 2026

View reviewed changes

added ']' for [0, pi] for _axis_angle.py and negative random vectors …

8f95e38

…as well for test_batch_rotations.py

Changed test_norm_axis_angle_180_degrees_deterministic_batch with lit…

436d623

…eral values for non random tests

AlexanderFabisch reviewed Sep 7, 2026

View reviewed changes

Add non-circular matrix-equivalence check for pi axis-angle normaliza…

2da5e86

…tion test

AlexanderFabisch reviewed Sep 15, 2026

View reviewed changes

Conversation

TabishShahMohsin commented Aug 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexanderFabisch commented Aug 17, 2026

Uh oh!

AlexanderFabisch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexanderFabisch Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexanderFabisch Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TabishShahMohsin Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TabishShahMohsin commented Sep 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

AlexanderFabisch Aug 27, 2026 •

edited

Loading

AlexanderFabisch Aug 27, 2026 •

edited

Loading

TabishShahMohsin Sep 2, 2026 •

edited

Loading