Repository navigation
Conversation
1880cd7 to
fedb16c
Compare
f28a76e to
5418cd4
Compare
5418cd4 to
8f17980
Compare
3301a22 to
4a47b11
Compare
8f17980 to
424caac
Compare
424caac to
7b68092
Compare
7b68092 to
8a64edd
Compare
RyanPCo
left a comment
There was a problem hiding this comment.
Automated adversarial review (read-only; findings were checked by a second skeptical pass before posting): 1 high, 1 medium, 2 low, 6 nit.
|
|
||
| ac_keys: | ||
| human_bimanual: "actions_cartesian" | ||
| human_bimanual: "actions_keypoints" |
There was a problem hiding this comment.
high (correctness): The human ac_key is now actions_keypoints, but the default evaluator still visualizes actions_cartesian. train_zarr_cartesian mounts eval_hpt -> viz/cartesian.yaml, so Human.viz_gt_preds looks up predictions["human_bimanual_actions_cartesian"] and raises KeyError on the first validation sanity step (e.g. model=hpt_bc_flow_mecka data=mecka). eval_pi.yaml has the same problem: it still uses _build_human_cartesian_revert_eef_frame_transform_list and pi_cartesian viz for human_bimanual. Have the human recipes select a keypoints evaluator (viz keypoints mode on actions_keypoints, plus _build_human_keypoints_revert_6d_wristframe_transform_list), and add a test that runs one validation step instead of ~evaluator.
| ] | ||
|
|
||
|
|
||
| def _build_human_keypoints_revert_6d_transform_list( |
There was a problem hiding this comment.
medium (dead-code): _build_human_keypoints_revert_6d_transform_list and _build_human_keypoints_revert_6d_wristframe_transform_list are only used by tests. eval_pi.yaml and eval_hpt_wrist.yaml still map human_bimanual to _build_human_cartesian_revert_eef_frame_transform_list, which splits actions_cartesian. With the new keypoint defaults (e.g. train_zarr_cartesian_pi + model=pi0.5_bc_mecka + data=mecka), that key is missing from the batch, so the cam-frame validation path breaks. Please either add a keypoint evaluator config (or a Human.get_revert_transform_list(mode) dispatch like Eva's) that points at the wristframe keypoint revert, or drop these builders until something uses them.
| # data.train_datasets.human_bimanual.resolver.key_map.include_ee_pose=false \ | ||
| # data.train_datasets.human_bimanual.resolver.transform_list.mode=cartesian_6d \ | ||
| # data.train_datasets.human_bimanual.resolver.transform_list.include_ee_pose=false | ||
| _target_: egomimic.pl_utils.pl_model.ModelWrapper |
There was a problem hiding this comment.
low (duplication): This file is a full copy of the old hpt_bc_flow_human.yaml body (trunk, encoders, augs, optimizer, scheduler), and it has already drifted: reverse_kl_samples: 8 is missing here. Consider defaults: [hpt_bc_flow_human, _self_] and override only dims.human_bimanual: {proprio: 20, action: 18}, ac_keys.human_bimanual: actions_cartesian, stem_specs.human_bimanual.state_keypoints: null and a state_ee_pose stem. That uses the null-stem drop this PR adds to HPTModel. The camera_transforms history comment can go too, since this PR deletes it from the parent.
| losses = losses[..., : self._packed_width(action, embodiment_id, ac_key)] | ||
| return losses.mean() | ||
|
|
||
| def _packed_width(self, action, embodiment_id, ac_key) -> int: |
There was a problem hiding this comment.
low (simplification): The packed width is a static property of each converter: the cartesian converters always _pad32, and HumanBimanualKeypoints already declares native_dim = 144. Instead of running a forward to32_norm_6d and caching it in self._packed_widths, add a packed_dim class attribute to the converters and read self.action_registry.get(emb, ac_key).packed_dim in _reduce_loss. Then _packed_widths and _packed_width can go.
| multitask: false | ||
| pretrained: false | ||
| pretrained_checkpoint: "" | ||
| pretrained_checkpoint: "" # TODO |
There was a problem hiding this comment.
nit (style): This file gains trailing whitespace (diffusion: true , human_bimanual: , whitespace-only blank lines) and a bare # TODO on pretrained_checkpoint. Please strip the whitespace and either drop the TODO or say what is pending.
| def transform(self, batch: dict) -> dict: | ||
| arr, is_tensor = _as_array(batch[self.action_key]) | ||
| layout = bimanual_keypoint_layout(arr.shape[-1]) | ||
| if layout is None or arr.shape[-1] != 144: |
There was a problem hiding this comment.
nit (simplification): The two inverse transforms derive offsets differently: KeypointsYPRToRot6D reads them from bimanual_keypoint_layout (and recomputes xyz on every loop iteration), while this one hard-codes o + 3 / o + 9. Pick one style for both and hoist the offsets out of the loop. The guard can be just if arr.shape[-1] != 144:, because a 144 width always has a layout.
| # the canonical 32-slot block layout. | ||
| action32 = converter.to32_norm_6d(action) | ||
| # the canonical block layout and zero-pad up to the model width. | ||
| action32 = pad_to_width(converter.to32_norm_6d(action), self.action_dim) |
There was a problem hiding this comment.
nit (magic-number): action32 now holds an action_dim-wide (e.g. 144) vector, so the name is misleading. Rename it (e.g. action_vec). Also consider an OPENPI_PRETRAINED_ACTION_DIM = 32 constant for the self.action_dim != 32 check, its log message and _pad32.
|
|
||
| # 144 = per hand [wrist xyz (3) | wrist rot6d (6) | 21 MANO keypoints (63)] | ||
| # x 2, the `keypoints_*_6d` transform modes (the 138-D ypr layout of the | ||
| # `keypoints_*_ypr` modes is no longer the default). |
There was a problem hiding this comment.
nit (comments): '(the 138-D ypr layout of the keypoints_*_ypr modes is no longer the default)' describes the change, not the config. Keep only the layout description, and drop the extra blank line added after dims.
| multitask: false | ||
| pretrained: false | ||
| pretrained_checkpoint: "" | ||
| pretrained_checkpoint: "" # TODO |
There was a problem hiding this comment.
nit (cleanliness): This file brings back trailing whitespace (diffusion: true , human_bimanual: , blank lines with spaces) and the "" # TODO that main had cleaned up. It also drops the comment explaining why camera_transforms: {} is still passed, which hpt_bc_flow_human_cartesian.yaml keeps. Restore main's formatting and that comment, and change only dims, ac_keys and the stem name.
| def transform(self, batch: dict) -> dict: | ||
| arr, is_tensor = _as_array(batch[self.action_key]) | ||
| layout = bimanual_keypoint_layout(arr.shape[-1]) | ||
| if layout is None or arr.shape[-1] != 144: |
There was a problem hiding this comment.
nit (simplification): layout is None or arr.shape[-1] != 144 reduces to arr.shape[-1] != 144. The two inverse transforms also index the same layout differently: KeypointsYPRToRot6D derives offsets from bimanual_keypoint_layout (and recomputes xyz inside the loop), while this one hard-codes o+3/o+9. Pick one style for both so they can't drift apart.
…an default
Human data trains on hand keypoints instead of a palm-origin cartesian pose.
The action is 144-D wrist-first, per hand
[wrist xyz (3) | wrist rot6d (6) | 21 MANO keypoints (63)] x {left, right},
built by the keypoints_{wrist,head}frame_6d modes: the existing ypr keypoint
builders produce the 138-D layout and KeypointsYPRToRot6D re-encodes just the
wrist rotation, leaving the keypoint blocks untouched. Both the action and the
observations.state.keypoints proprio are converted, so neither carries a
wrapping euler angle. MANO keypoint 0 IS the wrist, so per hand the wrist
position rides at both ch 0-2 and ch 9-11; that is kept deliberately (21
keypoints is the MANO interface every consumer reshapes to) and written down
in the bimanual_keypoint_layout docstring.
include_ee_pose additionally builds the cartesian head-frame ee_pose next to
the keypoint action (keymap + transform list), so a keypoint pi0.5 run
discretizes the same State: prompt block as the cartesian runs and differs
from them only in the action space. The cartesian builder runs with
delete_target_world=False to keep obs_head_pose alive for the keypoint
builder, and its actions_cartesian by-product is dropped.
144 does not fit openpi's action vector, whose PyTorch port hard-codes 32-wide
action projections regardless of Pi0Config.action_dim. PI now reads action_dim
and, when it is not 32, swaps in action_dim-wide action_in_proj /
action_out_proj AFTER the strict base-weight load: only those two layers start
fresh, everything else comes from the checkpoint, and sample_actions already
draws noise at config.action_dim. pad_to_width zero-pads narrower embodiments
(eva's 20-D) into the same vector and raises rather than truncating a wider
one. _reduce_loss then restricts the mean to the slots the converter actually
packs, so eva does not average its loss over 112 zero-padded dims and
contribute a fraction of the gradient signal it had at width 32.
HumanBimanualKeypoints is identity-in, slice-out.
HPT human configs move to a 144-D state_keypoints stem. hpt_bc_flow_human
holds the body -- there is no vendor notion at the embodiment level, so the
canonical config is the embodiment one -- and hpt_bc_flow_{mecka,aria,scale}
are two-line aliases of it. HPTModel.add_stem drops null stem entries so a
child config can delete a stem its base declares, which hydra merges cannot
express, and hpt_cotrain_flow_seperate_head uses it to drop state_ee_pose.
With the keypoints_*_6d modes in place, the guard from the previous commit
widens to the keypoints ypr and quat modes, so every remaining discontinuous
rotation target needs an explicit allow_legacy_rotation=True (the aria->MANO
and mecka keypoint viz tutorials, which read pre-6D data). The cartesian
configs that were still pinned to the ypr widths move with it:
hpt_bc_flow_human_cartesian and the cotrain base to human 20/18, the
shared-head cotrains to a 20-wide shared action.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01VBmhpVy7ZcMtsBJWh38cJp
8a64edd to
780940d
Compare
|
Review findings: status after the fixup pass. The fixes are squashed into this PR's commits, and the whole stack was force-pushed. Fixed
Partial
Not fixed
|

Human data trains on hand keypoints instead of a palm-origin cartesian pose.
The action is 144-D wrist-first, per hand
[wrist xyz (3) | wrist rot6d (6) | 21 MANO keypoints (63)] x {left, right},
built by the keypoints_{wrist,head}frame_6d modes: the existing ypr keypoint
builders produce the 138-D layout and KeypointsYPRToRot6D re-encodes just the
wrist rotation, leaving the keypoint blocks untouched. Both the action and the
observations.state.keypoints proprio are converted, so neither carries a
wrapping euler angle.
include_ee_pose additionally builds the cartesian head-frame ee_pose next to
the keypoint action (keymap + transform list), so a keypoint pi0.5 run
discretizes the same State: prompt block as the cartesian runs and differs
from them only in the action space. The cartesian builder runs with
delete_target_world=False to keep obs_head_pose alive for the keypoint
builder, and its actions_cartesian by-product is dropped.
144 does not fit openpi's action vector, whose PyTorch port hard-codes 32-wide
action projections regardless of Pi0Config.action_dim. PI now reads action_dim
and, when it is not 32, swaps in action_dim-wide action_in_proj /
action_out_proj AFTER the strict base-weight load: only those two layers start
fresh, everything else comes from the checkpoint, and sample_actions already
draws noise at config.action_dim. pad_to_width zero-pads narrower embodiments
(eva's 20-D) into the same vector and raises rather than truncating a wider
one. _reduce_loss then restricts the mean to the slots the converter actually
packs, so eva does not average its loss over 112 zero-padded dims and
contribute a fraction of the gradient signal it had at width 32.
HumanBimanualKeypoints is identity-in, slice-out.
HPT human configs move to a 144-D state_keypoints stem (hpt_bc_flow_mecka,
which human/aria/scale now alias); the cartesian variant survives as
hpt_bc_flow_human_cartesian. HPTModel.add_stem drops null stem entries so a
child config can delete a stem its base declares, which hydra merges cannot
express, and hpt_cotrain_flow_seperate_head uses it to drop state_ee_pose.
Shared-head cotrain configs need one action space and stay cartesian.
Co-Authored-By: Claude Opus 5 (1M context) [email protected]
🤖 Generated with Claude Code
https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw