6D rotation normalization to prevent large changes from euler

[RyanPCo]

normalization to prevent large changes from euler6D rotation

[RyanPCo]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• local: personal env overrides — Do not merge #493 : 2 dependent PRs (#491 , #494 )
• 6D rotation normalization to prevent large changes from euler #490 👈 (View in Graphite)
• remove rotation bounds checks for norm stats #480 : 1 other dependent PR (#481 )
• optional viz videos on train dataset #476
• optional flag to generate validation videos for each task #475
• pi train human new #473
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[github-actions]

Claude Code Review

Review of PR #490: 6D rotation normalization

Summary

Migrates Pi 0.5 action/proprio rotations from Euler ypr to continuous 6D (Zhou et al.) to eliminate ±π wrap and gimbal-lock issues that broke per-dim normalization. Adds numpy 6D pose helpers, new XYZWXYZ_to_XYZ6D / XYZ6D_to_XYZYPR transforms, cartesian_6d modes per embodiment, two new action converters, and a thorough test suite.

Key concerns

1. Silent default change in Eva revert (eva.py). _build_eva_bimanual_revert_eef_frame_transform_list adds rot_repr="ypr" with is_quat=True still the default. Previously is_quat=True produced pose_shape=7 with mode="xyzypr" — which was inconsistent already, but the new code path rot_repr == "ypr" collapses to pose_shape = 7 if is_quat else 6 and revert_mode="xyzypr". If anything was relying on the old is_quat=True default reverting from quat, it would break. Worth grepping callsites; eval_hpt_wrist.yaml was updated, good. Confirm no other callers pass is_quat=True expecting quaternion revert.

2. Mecka.get_transform_list previously called _build_aria_…. The diff replaces it with _build_human_cartesian_bimanual_transform_list. Was the old _build_aria_cartesian_bimanual_transform_list semantically equivalent for Mecka? If they differed (e.g., default stride/chunk_length, or head-pose handling), the legacy mode="cartesian" Mecka path now produces different actions. This silently affects mecka_pi_50_hrs.yaml if anyone falls back to cartesian mode. Please confirm or add a regression test.

3. mecka_pi_10_hrs.yaml task filter swap is unrelated to the PR title. Switching from 10 packaging/wrapping/etc. tasks to {cutting_fabric, hanging_clothes, brushing_shoes} is a big training-config change masquerading as a rotation PR. This will break anyone reproducing the "10 hrs" benchmark. Either split this out or rename the file.

4. check_val_every_n_epoch halved globally (default.yaml 100→50, ddp_pi.yaml 150→50). Doubles/triples validation cost across every run that inherits these. Not related to the PR title; consider extracting.

5. submitit_cpu_pace.yaml cpus_per_task 24→12 — unrelated infra change buried in a rotation PR. Easy to miss in review and could regress data processing throughput.

6. print_valid_annotations.py is a debug script committed to egomimic/scripts/. Fine, but it has hard-coded paths (/storage/project/r-dxu345-0/...) and a hard-coded log directory in the docstring. Consider moving to a scripts/debug/ or tools/ location, or at minimum noting it's a one-off.

7. Norm stats invalidation. Existing checkpoints trained on cartesian (ypr) cannot be loaded with cartesian_6d configs — different action width (12→18, 14→20) and different unnormalize semantics. The PR doesn't add a migration note or version bump. Suggest documenting in the PR description and/or model config that 6D configs require fresh training.

Suggestions

• Split this PR: (a) 6D rotation infra + tests + opt-in configs, (b) the mecka_pi_10_hrs.yaml task swap, (c) trainer val-frequency and slurm cpu changes. Each is independently reviewable.
• Add an end-to-end regression test that loads an old cartesian config and confirms it still produces 12D/14D outputs (i.e., the legacy path is untouched).
• For _build_eva_bimanual_revert_eef_frame_transform_list, consider making rot_repr the single source of truth and deprecating is_quat to avoid the two overlapping flags. Right now rot_repr="ypr" + is_quat=True is a legal but probably-wrong combination.
• In bimanual_cartesian_layout, the rot indices for width 18/20 include all 6 columns per arm (12 total). The eval _split_mse will average MSE over 12 rotation channels for 6D vs 6 ypr channels for legacy — that's fine in principle, but the dashboard key _ypr_ is now misleading for 6D runs. Consider renaming the metric key with a deprecation alias, or at least documenting the scale change so people don't compare 6D rotation MSE numbers to historical ypr MSE numbers.
• Tests are excellent — particularly the numpy↔torch Gram-Schmidt parity test and the revert-equivalence tests. Consider adding one test that exercises ActionChunkCoordinateFrameTransform(mode="xyz6d", inverse=True) since the revert path uses inverse=False but other paths may use inverse=True.
• The torch _reconstruct_R_from_cols and numpy _xyz6d_to_matrix are now duplicate logic. Add a comment cross-referencing both, or factor eps=1e-8 into a shared constant so they can't drift.

Verdict: Request Changes

The 6D infrastructure itself is well-engineered, well-tested, and correct. But the PR bundles unrelated training-config changes (mecka task filter swap, val frequency, slurm CPUs) that are easy to miss and will affect other people's runs. Please split those out, and clarify the Mecka transform-list switch (_build_aria_… → _build_human_…) is behavior-preserving.

---

Reviewed by Claude · Review workflow