Fix quaternion convention in locomanip SDG lerobot conversion dataset#5754
Fix quaternion convention in locomanip SDG lerobot conversion dataset#5754jaybdub wants to merge 1 commit into
Conversation
… locomanipulation SDG
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
PR #5754: Fix quaternion convention in locomanip SDG lerobot conversion dataset
Summary
This PR fixes a quaternion convention mismatch in the locomanipulation SDG dataset conversion script. After PR #4437 changed Isaac Lab from WXYZ to XYZW convention (Jan 2026), this script was still using scalar_first=True when calling scipy's Rotation.from_quat() and as_quat(), causing all converted datasets to have corrupted pose data.
Analysis
✅ Correctness of the Fix
The fix is correct. Here's the breakdown:
| Convention | Quaternion Order | scipy scalar_first |
|---|---|---|
| WXYZ (old Isaac Lab) | (w, x, y, z) | True |
| XYZW (current Isaac Lab) | (x, y, z, w) | False (default) |
Since Isaac Lab now uses XYZW (scalar-last) and scipy's from_quat() / as_quat() default to scalar_first=False, removing the explicit scalar_first=True parameter correctly aligns the code with the new convention.
✅ Documentation Updates
The docstrings are properly updated:
pose_to_transform: Updated to document "scalar-last (x, y, z, w) order" and referencesisaaclab.assets.ArticulationData.body_link_pose_wpose_from_transform: Updated to match the same convention documentation
⚠️ Minor Observations
-
No regression test included: The PR description checklist shows testing is not checked. For a bug fix affecting training data quality, a unit test verifying the conversion matches expected output would be valuable.
-
Downstream impact: Since this changes the output format for all converted datasets, any existing converted datasets would need to be regenerated. This is expected behavior for a bug fix.
Checklist Review
| Item | Status |
|---|---|
| Bug fix classification | ✅ Correct |
| Quaternion math | ✅ Correct |
| Docstrings | ✅ Updated |
| Pre-commit checks | ⏳ Not confirmed in checklist |
| Tests added | ❌ Not added |
| Changelog updated | ❌ Not checked |
Recommendation
The core fix is technically correct and addresses a real data corruption issue. Consider adding:
- A simple unit test to guard against future convention regressions
- Changelog fragment for this bug fix
This review was generated by the Isaac Lab Review Bot. CI checks are currently pending.
Greptile SummaryThis PR fixes a quaternion convention mismatch in
Confidence Score: 5/5The change is a two-line fix with no side-effects — it removes an incorrect flag from two scipy calls and updates matching docstrings. The rest of the codebase (rollout_policy.py) already operates with XYZW quaternions, confirming the direction is correct. Both the conversion direction (from_quat) and output direction (as_quat) are updated consistently. The rollout script independently establishes that the environment emits XYZW and defaults to that format, corroborating the fix. No other files in the scripts/imitation_learning tree use scalar_first. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["HDF5 Dataset\n(Isaac Lab XYZW poses)"] --> B["pose_to_transform()\nRotation.from_quat(q)\ndefault: scalar_first=False = XYZW ✅"]
B --> C["RigidTransform\nbatch Rotation"]
C --> D["compute_relative_pose()\nbase_pose.inv() * target"]
D --> E["pose_from_transform()\nrotation.as_quat()\ndefault: scalar_first=False = XYZW ✅"]
E --> F["7D pose array\n[x,y,z, x,y,z,w]"]
F --> G["Parquet / GR00T\nTraining Dataset"]
Reviews (1): Last reviewed commit: "fix quaternion scalar_first=True bug in ..." | Re-trigger Greptile |
Description
scripts/imitation_learning/locomanipulation_sdg/gr00t/convert_dataset.pystill passed
scalar_first=TruetoRotation.from_quat/as_quat, butsince #4437 (Jan 2026) Isaac Lab returns body poses in XYZW. The script
therefore mis-interpreted recorded poses as WXYZ, corrupting every
relative-pose computation (hands, object, fixture, goal) in the GR00T
training data and causing trained policies to produce twisted-arm
rollouts.
Type of change
Screenshots
Please attach before and after screenshots of the change if applicable.
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there