jmni/zarr_registry

[jaynye]

Adds a registry for end effector+base factorized embodiments and defines a new transform naming convention A_T_B (frame B to frame A) to remove ambiguity when dealing with robot data

[github-actions]

Claude Code Review

Review

Summary

Introduces a YAML-based embodiment registry (platforms + end-effectors + aliases) and renames coordinate-frame variables/methods to follow the A_T_B convention. Also adds CI, docs, and a deprecated-name alias shim for cached zarr episodes.

Key concerns

1. Alias shim only works on read, not write — test_aliases_are_a_read_shim_only confirms ZarrWriter still rejects aria_bimanual. That's the intended contract, but be aware: any scripts still passing legacy vendor names to writers will break. Worth grepping egomimic/scripts/ for aria_bimanual, mecka_bimanual, scale_bimanual string literals before merge — the diff doesn't touch them.

2. ARIA_T_RGB_CPF → ARIA_RGB_T_CPF rename — this is a semantic reversal in the new convention (rgb ← cpf vs. old aria "T from rgb to cpf"). Please verify against the original Aria SDK matrix that the underlying numeric values still mean "map CPF → RGB". If the original was already RGB_T_CPF under the new convention (i.e., p_rgb = M @ p_cpf), this is correct. If not, the gaze viz will silently render in the wrong frame. This matrix is used only in viz, so no training regression, but worth a manual check by someone who knows the Aria SDK convention.

3. Registry says reference_frame: camera:front_1 for EVA, but docs say extrinsics are base_T_cam — these are consistent (extrinsics express camera pose in base frame, and calibration reference is the front camera), but the semantics of reference_frame in platforms.yaml are underdocumented. What is it used for downstream? Currently nothing in the diff consumes it — is this a future hook? If unused, drop it or add a docstring pointing at the consumer.

4. Cached EMBODIMENT_CLASSES at import time — _build_embodiment_classes() runs during egomimic.rldb.embodiment import and calls _import_embodiment_class for every platform. This introduces an import-time dependency chain: importing the embodiment package now imports Eva and Human transitively via string paths. Fine now, but if a platform ever lists a class with heavy deps (torch models, etc.), it will slow all imports. Consider lazy resolution in get_embodiment_class.

5. ResolvedEmbodiment.get_keymap / get_transform_list delegate to class, ignoring end-effector selection — the test test_resolve_delegates_to_the_hand_written_pipeline shows eva.get_transform_list(...) == Eva.get_transform_list(...). So resolving {platform: eva_x5, end_effector: mano_hand} returns the jaw transform pipeline. This makes the "swap end-effectors without code" promise (test name test_a_new_hand_on_a_known_platform_costs_no_code) misleading — the registry lets you declare the swap, but the pipeline is still hand-written per class. Recommend either (a) documenting this limitation clearly in ResolvedEmbodiment docstring, or (b) renaming the test to reflect that only metadata is registry-driven today.

Suggestions

• Add a migration test that loads a real cached zarr with embodiment="aria_bimanual" in attrs and confirms training data loading still works end-to-end (not just get_embodiment_id).
• Document what reference_frame in platforms.yaml will be used for, or drop it until a consumer exists.
• In CONVENTIONS.md, add one sentence stating this is a naming-only migration — no stored data changes — since that's the load-bearing claim.
• Consider adding ruff check to pre-commit given CI now enforces it.
• The variable renames in ActionChunkCoordinateFrameTransform.transform (chunk_se3 → chunk_pose_transforms, etc.) are verbose but clearer. Fine.
• Minor: _build_eva_bimanual_eef_frame_transform_list and _build_eva_bimanual_transform_list have duplicated extrinsics → base_T_cam_pose conversion logic. Consider a small helper to reduce drift risk.

Verdict: Comment

The registry design is solid and the naming convention doc is welcome. Before approving I'd want:

1. Confirmation that no writer scripts still pass deprecated names (grep result).
2. Confirmation on the ARIA_RGB_T_CPF semantic direction against the Aria SDK.
3. A real cached-episode load test to prove the alias shim works end-to-end.

None of these should block landing if verified out-of-band, but they aren't visible in this diff.

---

Reviewed by Claude · Review workflow