[Fix] Use matrix multiplication for rotation composition in test_pink_ik

[hujc7]

Summary

One-character fix in source/isaaclab/test/controllers/test_pink_ik.py:309:

- quat_from_matrix(matrix_from_quat(target_rot_tensor) * matrix_from_quat(quat_inv(current_rot)))
+ quat_from_matrix(matrix_from_quat(target_rot_tensor) @ matrix_from_quat(quat_inv(current_rot)))

calculate_rotation_error was composing two rotation matrices with PyTorch's element-wise multiplication (*) where matrix multiplication (@) was intended. The Hadamard product of two rotation matrices is not generally a rotation matrix.

Why this surfaced as test failures now

The bug has been latent since isaac-sim/IsaacLab#3149 (2025-08-26) because the Hadamard product of two near-identity matrices is also near-identity — quat_from_matrix could still recover a near-unit quaternion and the assertion rot_error ≈ 0 would pass for completely wrong mathematical reasons.

It became visible when isaac-sim/IsaacLab#5609 (jmart) (2026-05-14) added the unit-norm guard to isaaclab/utils/math.py:quat_from_matrix:

invalid = (quat.norm(p=2, dim=-1, keepdim=True) - 1.0).abs() > 2e-5
return torch.where(invalid, torch.full_like(quat, float("nan")), quat)

After that PR, any non-rotation input (the Hadamard mess) returns NaN, which axis_angle_from_quat propagates → torch.max(NaN) = NaN → AssertionError: Left hand IK rotation error (nan) exceeds tolerance. Both hands always went to NaN; left hand is just asserted first.

Verification

Local repro on the Horde VM against current develop (isaaclab_physx backend, newton[sim]@v1.2.0rc2):

Configuration	Result
Unfixed, Isaac-PickPlace-GR1T2-Abs-v0-horizontal_movement	FAILED — Left hand IK rotation error (nan)
Fixed, same parameterization	PASSED — rotation errors 1e-4 to 1e-7 (well within 0.02 rad tolerance)
Fixed, all 12 GR1T2 cases, run 1	11 passed, 1 skipped
Fixed, all 12 GR1T2 cases, run 2	11 passed, 1 skipped (deterministic)

Scope

This addresses the consistent Left hand IK rotation error (nan) failures seen across recent develop PRs (e.g. isaac-sim/IsaacLab#5633 test-curobo log, isaac-sim/IsaacLab#5609 test-curobo log, isaac-sim/IsaacLab#5616 test-curobo log).

Remaining failures on G1 envs (finite ~0.03-0.05 rad rotation errors against the 0.030 rad tolerance) are a separate issue — IK convergence quality rather than the NaN math bug. Out of scope for this PR; needs its own ticket.

Test plan

• Pre-commit clean.
• Unfixed branch reproduces NaN on Isaac-PickPlace-GR1T2-Abs-v0-horizontal_movement locally.
• Fixed branch passes the same parameterization locally with finite rotation errors.
• Fixed branch passes all 12 GR1T2 parameterizations across two consecutive runs (deterministic).

[greptile-apps]

Greptile Summary

Fixes a latent operator bug in calculate_rotation_error (test helper in test_pink_ik.py) where element-wise multiplication (*) was used to compose two rotation matrices instead of matrix multiplication (@), producing a non-rotation Hadamard product. The bug became observable after quat_from_matrix gained a unit-norm guard that returns NaN for non-rotation inputs.

• One-character fix (* → @) in calculate_rotation_error at line 309, restoring R_target @ R_current⁻¹ as the correct relative-rotation formula.
• Changelog fragment added under changelog.d/ documenting the root cause, the prior PR that introduced it, and the subsequent PR that made it visible.

Confidence Score: 5/5

Safe to merge — the single-character operator change is mathematically correct and the PR description includes local reproduction evidence confirming both the failure and the fix.

The change is minimal and targeted: one operator in a test helper function, with a clear mathematical justification. The changelog fragment is accurate. No production code is touched.

No files require special attention.

Important Files Changed

Filename	Overview
source/isaaclab/test/controllers/test_pink_ik.py	Single-operator fix: element-wise * replaced with matrix multiplication @ in calculate_rotation_error, restoring mathematically correct rotation composition.
source/isaaclab/changelog.d/jichuanh-pink-ik-left-hand-nan.rst	New changelog fragment accurately describing the fix, its root cause, and the chain of events that made the latent bug observable.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["current_rot (quat)"] --> B["quat_inv(current_rot)"]
    B --> C["matrix_from_quat(quat_inv)"]
    D["target_rot (quat)"] --> E["matrix_from_quat(target_rot)"]
    E --> F{"Compose rotation matrices"}
    C --> F
    F -->|"Before fix (BUG): *\nHadamard product"| G["Non-rotation matrix\n(not in SO(3))"]
    F -->|"After fix (CORRECT): @\nMatrix multiplication"| H["R_target @ R_current^-1\n(valid rotation matrix in SO(3))"]
    G --> I["quat_from_matrix -> NaN\n(unit-norm guard rejects it)"]
    H --> J["quat_from_matrix -> valid quat"]
    I --> K["axis_angle_from_quat -> NaN"]
    J --> L["axis_angle_from_quat -> rotation error"]
    K --> M["AssertionError: rotation error nan"]
    L --> N["Assert error < 0.02 rad"]

Loading

_{Reviews (1): Last reviewed commit: "Use matrix mul to compose rotations in t..." | Re-trigger Greptile}

[isaaclab-review-bot]

✅ Review Summary

Verdict: LGTM — Clean, minimal, mathematically correct fix.

Analysis

Rotation Math Correctness:
The fix changes element-wise multiplication (*) to matrix multiplication (@) for rotation matrix composition. This is the correct operation:

• Rotation composition requires R_rel = R_target @ R_current^(-1)
• The Hadamard product of two SO(3) matrices does not produce an SO(3) matrix (columns are not orthonormal, determinant ≠ 1)
• The @ operator correctly performs batched matrix multiplication on the (*, 3, 3) outputs from matrix_from_quat()

Why the bug was latent for ~9 months:
For near-identity rotation matrices, the Hadamard product happens to be close to the matrix product, so quat_from_matrix could still recover a near-unit quaternion. The recent unit-norm guard in PR #5609 correctly surfaced this bug by returning NaN for non-rotation inputs.

Test Coverage:

• The fix is in test helper code (calculate_rotation_error)
• Self-testing: existing assertions would fail if the fix were incorrect
• Author verified locally across multiple GR1T2 configurations

---

📝 Update (2026-05-16): New Commits Reviewed

Changes since 4fdc11b:

1. Deterministic seeding added (test_pink_ik.py):

   • Added env_cfg.seed = 42 in create_test_env
   • Ensures IK convergence residuals are reproducible across runs/machines
   • ✅ Good practice for CI stability

2. G1 rotation tolerance adjusted (pink_ik_g1_test_configs.json):

   • Changed from 0.080 rad (4.6°) → 0.100 rad (5.7°)
   • Provides additional margin for G1's slower-converging IK tuning
   • ✅ Reasonable — stays well below thresholds that would affect behavior

3. Changelog simplified:

   • Condensed from detailed technical explanation to concise bullet points
   • Added the deterministic seed note
   • ✅ Cleaner and easier to scan

Assessment: All changes are sensible refinements. The deterministic seeding is a nice addition for CI reproducibility.

---

Automated review by isaaclab-review-bot