Repository navigation
Conversation
4e3e283 to
b097f9e
Compare
61d2de0 to
6f6ac72
Compare
b097f9e to
8864ceb
Compare
6f6ac72 to
1dbed28
Compare
af0f481 to
d28b3f8
Compare
2a02e81 to
afc17c5
Compare
d28b3f8 to
98b98e3
Compare
RyanPCo
left a comment
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, 3 low, 2 nit.
| return self.loss_fn(pred, target, pad_mask=data.get("pad_mask")) | ||
| # Unlike the pad mask, always applied: an untracked hand's action channels | ||
| # hold fill values, not motion. | ||
| mask = _with_hand_mask( |
There was a problem hiding this comment.
high (correctness): Under HPT this always gets None. HPT._robomimic_to_hpt_data never copies action_hand_mask from the batch into data, so HPT heads are still trained on the identity-pose fill of an untracked hand. Copy the key in _robomimic_to_hpt_data, and add a test that runs HPT's compute_loss with a left-only mask and checks that changing the right hand's target leaves the loss unchanged.
|
|
||
| HAND_FLAGS = {"action_keys": "action_hand_mask", "proprio_keys": "proprio_hand_valid"} | ||
| # Norm-sample key holding the hand_keep mask of norm key k, as f"{k}{KEEP_SUFFIX}". | ||
| KEEP_SUFFIX = "#keep" |
There was a problem hiding this comment.
low (simplification): The hand-keep masks ride in collected under f"{k}#keep" keys, so every consumer has to know to pop them before treating collected as {norm_key: samples}. KEEP_SUFFIX is also imported into episode_norm_samples and written into its cache rows. Returning the masks as a separate keeps dict from _collect_norm_samples / collect, aligned with collected[k], would let you drop KEEP_SUFFIX and keep the dict type dict[str, list[np.ndarray]].
| channels = hand_channel_mask( | ||
| hand_mask.to(actions.device, actions.dtype), actions.shape[-1] | ||
| ) | ||
| if channels is None: |
There was a problem hiding this comment.
low (duplication): This None-layout branch (return no mask if every hand is tracked, else raise 'action width ... has no bimanual layout ...') is duplicated verbatim in PI._packed_hand_mask. Consider a pose_utils helper (e.g. require_hand_channel_mask(hands, width)) used by both, so the rule and the message stay in one place.
| return batch | ||
| return { | ||
| **batch, | ||
| ac_key: torch.where(keep[:, None, :] > 0.5, gt, pred.to(gt.device, gt.dtype)), |
There was a problem hiding this comment.
low (correctness): Copying predictions into the untracked GT channels zeroes their error, but MSE, Frechet and layout_metrics still average over those channels. Validation error is therefore deflated in proportion to how often a hand is untracked, and runs with different require_hands_in_frame settings are not comparable. Weight the metrics by the hand channel mask (masked error sum / mask sum), and use the fill only for the viz transforms.
|
|
||
| @staticmethod | ||
| def _compute_stats_for_array(X): | ||
| if np.isnan(X).any(): |
There was a problem hiding this comment.
nit (duplication): The NaN branch duplicates all nine stats, differing only in np.X vs np.nanX. Pick the ops once (e.g. mean, std, ... = (np.nanmean, ...) if np.isnan(X).any() else (np.mean, ...)) and build the dict a single time so a new stat cannot drift between the two paths.
| # An untracked hand's channels hold fill values; the loss and the | ||
| # stats leave them out, so they cannot reject a sample either. | ||
| keep = None | ||
| if any(f in data for f in HAND_FLAGS.values()): |
There was a problem hiding this comment.
nit (simplification): hand_keep already returns None without flags, so the any(f in data ...) pre-check is redundant. The torch.ones_like(arr, dtype=bool) fallback allocates a full mask per key per sample on every flagless (zarr) sample. Consider keeping keep=None and applying & keep only when it is set, e.g. bad = ~torch.isfinite(arr); if keep is not None: bad &= keep, and likewise for below/above.
…unds Samples with action_hand_mask / proprio_hand_valid (Lance leaves) now: - HPT/RDT (DenoisingPolicy) multiply the pad mask by the hand's channels of the bimanual layout, whatever use_pad_mask says; aux heads see the same flags. - Pi takes a masked mean over the converter's packed slots; slots the converter inserts (gripper zeros, padding) stay in, so an all-tracked batch gives today's loss. - Norm stats set untracked cells to NaN and use nan-aware stats per cell, in both the DataLoader and the per-episode cache paths; the bounds check skips those cells. - Proprio of an untracked hand reads as the dataset mean pose: the normalized mean, so 0 on normalized channels and the mean on rot6d pass-through ones. - HPT and Pi eval take an untracked hand's ground truth from the prediction, so it adds no error and the viz revert never sees the zero fill rotation. Claude-Session: https://claude.ai/code/session_01Kt1E8T2x8kdV9akAvbJyVd
98b98e3 to
ffd10b3
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
|
Samples with action_hand_mask / proprio_hand_valid (Lance leaves) now:
the bimanual layout, whatever use_pad_mask says; aux heads see the same flags.
converter inserts (gripper zeros, padding) stay in, so an all-tracked batch
gives today's loss.
both the DataLoader and the per-episode cache paths; the bounds check skips
those cells.
mean, so 0 on normalized channels and the mean on rot6d pass-through ones.
so it adds no error and the viz revert never sees the zero fill rotation.
https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw