refactoring aciton transforms

[AnikethCheluva]

No description provided.

[AnikethCheluva]

• refactoring aciton transforms #604 : 2 dependent PRs (#176 , #605 ) 👈 (View in Graphite)
• main

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

[github-actions]

Claude Code Review

Review of PR #604: Refactoring Action Transforms

Summary

Refactors get_transform_list from a single mode string enum to three orthogonal axes (action_mode, coord_frame, rotation_mode), unlocking a new Zhou 6D rotation representation and generalizing gripper padding. Updates all Hydra data configs and downstream call sites to match.

Key concerns

1. .ipynb cell outputs committed with binary video blob

egomimic/scripts/tutorials/zarr_data_viz.ipynb still contains a huge base64-encoded video/mp4 payload (~90k+ chars of the diff). The other cells were properly cleared (execution_count: null), but this one was missed. This is exactly the kind of thing your new .gitignore philosophy is trying to prevent — please strip all outputs before merging.

2. cotrain_pi_pickplace_qwen.yaml valid_datasets is missing the human split refactor

The train side has both eva and human entries updated, but I only see eva updated under valid_datasets in the diff (the human valid entry that likely exists is not shown updated). Please double-check that no mode: ... strings remain in any config — a stale mode: key will now be silently ignored by Hydra (unknown kwarg to get_transform_list → TypeError) or, worse, cause a mismatch between train and valid transforms.

Run: rg "mode:\s*(cartesian|keypoints)" egomimic/hydra_configs/

3. Behavior change in _build_human_keypoints_bimanual_transform_list when concat_keys=False

Before: rotation-mode conversion (BatchQuaternionPoseToYPR, etc.) ran regardless of concat_keys. After: the transforms_for_rotation_mode(...) call was moved inside the if concat_keys: block. This means callers with concat_keys=False (notably _build_human_keypoints_eef_frame_transform_list, which forces rotation_mode="quat" and then re-applies conversion later) get correct behavior by coincidence, but any future caller passing concat_keys=False + non-quat rotation will silently skip conversion. This is a footgun.

Suggest either:

• Keep rotation conversion outside the concat_keys gate (matches old semantics), or
• Add an explicit assert: assert concat_keys or rotation_mode == "quat".

4. PadGripperZeros for quat rotation mode is not supported

_pad_human_cartesian_gripper computes pose_dim for euler=6, quat=7, 6D=9. For quat, that's 7D per arm → 14D total → pad to 16D. That's mathematically fine, but the docstring on PadGripperZeros still says "matching the canonical [L xyz ypr g, R xyz ypr g] layout used by Eva". Update the docstring, and confirm you actually have a cartesian_gripper_padded + quat config path exercised anywhere (I don't see one in the diff — likely dead branch for now, which is fine, but flag it).

5. Norm-stats cache invalidation

Action dim/layout is unchanged for existing configs (all migrations preserve the same output tensor: cartesian+camframe+euler = old cartesian, etc.), so cached norm stats should still be valid. But please verify by running one existing config end-to-end and confirming that norm_stats_cache/ hits produce identical values to a fresh recompute. A silent norm-stats corruption here would poison every downstream run.

6. industry_eva_pi.yaml and mecka_pi.yaml etc. previously had no mode: field

Several *_pi.yaml configs previously relied on defaults from get_transform_list (no mode in yaml). The old signature had mode as a required positional-ish arg with no default — how did these work before? Either they were broken, or the old defaults matched cartesian. Now they explicitly get action_mode: cartesian, coord_frame: camframe, rotation_mode: euler. Confirm this matches the prior implicit behavior; if not, this is a silent training regression for Pi 0.5 runs.

Suggestions

• Clear the notebook outputs: jupyter nbconvert --clear-output --inplace egomimic/scripts/tutorials/zarr_data_viz.ipynb
• Add a ValueError for the currently-unreachable combos (action_mode="keypoints" + cartesian_gripper_padded) to make the API self-documenting.
• Add a unit test that instantiates get_transform_list for each (action_mode, coord_frame, rotation_mode) combo and asserts the output tensor dim (6/7/9 × arms + gripper slots). This makes future rotation representations (e.g., axis-angle) trivial to add.
• Grep for the old mode strings across the repo — I'd expect cartesian_wristframe_ypr, keypoints_headframe_ypr, cartesian_padded etc. to appear only in docstrings/comments now:

  rg "cartesian_wristframe_ypr|keypoints_headframe_ypr|keypoints_wristframe_ypr|cartesian_padded"
  

• Consider a shim: if someone passes mode= (old kwarg), raise a TypeError with a clear migration message rather than the default "unexpected keyword argument".

Verdict

Request Changes — primarily to (1) clear the notebook video blob, (2) verify no stale mode: strings remain in configs, (3) fix the concat_keys=False conditional-rotation regression, and (4) confirm norm-stats cache compatibility on at least one live config. The refactor itself is a clean improvement — orthogonal axes are much easier to reason about than the exploding enum.

---

Reviewed by Claude · Review workflow