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.

+    # 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


+    # 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):


AlexanderFabisch · 2026-08-17T07:03:43Z


+    # 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


This is what I thought too. Please change the condition. isclose matches too broadly.

+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])


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:03:43Z


+    # 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


This is what I thought too. Please change the condition. isclose matches too broadly.

AlexanderFabisch · 2026-08-17T07:07:01Z


+    # 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):


Same here. isclose matches to broadly. The modification should just apply to exactly pi.

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.

AlexanderFabisch · 2026-08-17T07:27:58Z

+
+
+def test_norm_axis_angle_180_degrees_deterministic_batch():
+    A = np.array([


I'd add additional tests for [0, 0, 0] and random axes.

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, )

AlexanderFabisch · 2026-08-17T07:32:32Z

+    # the first non-zero component of axis should be positive.
+    pi_mask = np.isclose(res[..., 3], np.pi) & rot_mask
+    if np.any(pi_mask):
+        axes = res[pi_mask, :3]


The variable name axes is very generic. These are not all axes, but the axes for which the rotation is pi. I'd suggest renaming to axes_with_pi_rotation.

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.

AlexanderFabisch · 2026-08-17T07:35:12Z

        pbr.smooth_quaternion_trajectory(np.zeros((0, 4)))
+
+
+def test_norm_axis_angle_180_degrees_deterministic_batch():


Please also add test cases with other array shapes:

shape (4,)

shape (N, M, 4)

AlexanderFabisch · 2026-08-17T07:36:23Z

    )
+
+
+def test_norm_axis_angle_180_degrees_deterministic():


Same here, please add tests for [0,0,0] and random axes.

Done! I've added many test cases and tried to make them consistent with other test functions.

AlexanderFabisch · 2026-08-17T07:38:49Z

+    # the first non-zero component of axis should be positive.
+    if np.isclose(angle, np.pi):
+        is_zero = np.isclose(res[:3], 0.0)
+


I wouldn't put a blank line here. The only purpose of is_zero is to compute non_zero_indices, so they are conceptually connected and, hence, should be so spatially.

Have now removed it.

…ic_batch

…nt with others

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



		def test_norm_axis_angle_180_degrees_deterministic_batch():
		A = np.array([

		pbr.smooth_quaternion_trajectory(np.zeros((0, 4)))


		def test_norm_axis_angle_180_degrees_deterministic_batch():

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlexanderFabisch commented Aug 17, 2026

Uh oh!

AlexanderFabisch left a comment

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!

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 Aug 26, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

TabishShahMohsin Aug 26, 2026 •

edited

Loading