Fix Batch Solver Review Feedback

[zhx06]

Summary

Address the comments left in Batch Suport for Relation Solver

Detailed description

• Remove "(useful for deterministic evaluation)"
• Convert NOTE to TODO(zhx06)
• Fix quaternion order according to Lab 3.0

[greptile-apps]

Greptile Summary

This PR addresses review feedback from the batch solver PR (#512) with three targeted cleanups: removing a redundant parenthetical phrase from a docstring, converting a NOTE comment to a TODO(zhx06), and updating quaternion handling to match Isaac Lab 3.0's new XYZW convention (switched from WXYZ). The quaternion change is consistent throughout — quat_from_euler_xyz now returns (x, y, z, w) and euler_xyz_from_quat now expects (x, y, z, w) in Lab 3.0, so all call-sites in relations.py, object_placer.py, and pose.py align correctly with the rotation_xyzw fields in Pose.

Confidence Score: 5/5

Safe to merge — all three changes are narrow, well-scoped cleanups with no new logic introduced.

All findings are P2 or lower. The quaternion convention update is correct and consistent throughout the three files, aligning with Isaac Lab 3.0's XYZW-ordering change. Comment and docstring edits are cosmetic improvements. No behavioral regressions are expected.

No files require special attention.

Important Files Changed

Filename	Overview
isaaclab_arena/relations/relations.py	NOTE → TODO(zhx06) conversion on the NoCollision symmetric-pair concern; quaternion convention updated in RandomAroundSolution.to_pose_range_centered_at and RotateAroundSolution.get_rotation_xyzw to align with Lab 3.0 XYZW ordering — looks correct.
isaaclab_arena/relations/object_placer.py	No functional changes visible; quaternion identity default (0.0, 0.0, 0.0, 1.0) correctly represents XYZW identity for Lab 3.0 — no issues found.
isaaclab_arena/assets/object_base.py	Quaternion-related docstrings updated to document (x, y, z, qx, qy, qz, qw) XYZW ordering; init_state.rot assignment from rotation_xyzw now correctly aligns with Lab 3.0 XYZW convention.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["RotateAroundSolution.get_rotation_xyzw()"] -->|"quat_from_euler_xyz(roll, pitch, yaw)"| B["quat tensor — Lab 3.0 XYZW format"]
    B -->|"tuple(quat.tolist())"| C["rotation_xyzw (x, y, z, w)"]

    D["RandomAroundSolution.to_pose_range_centered_at(rotation_xyzw)"] -->|"torch.tensor([rotation_xyzw])"| E["quat tensor shape (1,4) XYZW"]
    E -->|"euler_xyz_from_quat — Lab 3.0 expects XYZW"| F["roll, pitch, yaw"]
    F --> G["PoseRange with RPY center ± half-extents"]

    C --> H["ObjectPlacer._apply_positions"]
    G --> H
    H -->|"Pose(rotation_xyzw=...)"| I["object.set_initial_pose()"]
    I -->|"init_state.rot = rotation_xyzw"| J["Isaac Lab 3.0 sim — expects XYZW"]

Loading

_{Reviews (1): Last reviewed commit: "address feedback" | Re-trigger Greptile}

[cvolkcvolk]

Thanks for the follow up!

[kellyguo11]

Clean follow-up to #512. All three changes are correct:

1. object_base.py — Docstring quaternion order fixed from (qw, qx, qy, qz) to (qx, qy, qz, qw). Matches the actual root_pose_w data layout in IsaacLab 3.0 and the Pose.rotation_xyzw convention used throughout the codebase.

2. object_placer.py — Removed parenthetical "(useful for deterministic evaluation)" from result_per_env docstring. This was flagged in review of #512 — the phrasing was misleading since the solver is already stochastic.

3. relations.py — NOTE: → TODO(zhx06): on the deduplication caveat in NoCollision. Properly attributes ownership for the follow-up work.

LGTM.