Repository navigation
Conversation
cc32016 to
fc6258a
Compare
4ab3149 to
e9445a0
Compare
fc6258a to
ad2cc51
Compare
e9445a0 to
c0a093d
Compare
b205bea to
f9a08b1
Compare
4c4c8a5 to
a88062e
Compare
a88062e to
e38a6d9
Compare
11cbb10 to
868598a
Compare
2158d8c to
dbdd171
Compare
133be3f to
e9ac33c
Compare
dbdd171 to
51525b5
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): 4 low, 2 nit.
| # (InterpolatePadMask) to the model horizon. | ||
| data["action_pad_mask"] = pad_mask | ||
|
|
||
| history_mask = self._proprio_history_mask(idx) |
There was a problem hiding this comment.
low (dead-code): proprio_history_mask is built on every __getitem__ (_proprio_history_mask rescans the keymap each time), carried through transforms and collate, and then discarded by the only consumer, hpt.py:1127. Either use it in the stem or drop it. Dropping it means removing _proprio_history, _proprio_history_mask, this emit block, the pop in hpt.py, the docstring mention in Human.get_keymap and the tests that only check the mask. It can come back when something reads it.
| # per-channel (D,) stats, so a K > 1 run and the K = 1 baseline | ||
| # share exactly one set of stats. Same rule as | ||
| # scripts/precompute_norm_stats.py. | ||
| km["proprio_history"] = 1 |
There was a problem hiding this comment.
low (duplication): This K=1 override is duplicated in scripts/precompute_norm_stats.py, and any other norm-stat caller would have to remember it too. Human.get_keymap already receives norm_mode and already strips keys based on it. Add if norm_mode: proprio_history = 1 there with a one-line reason, and delete both call-site blocks.
| lang_keys, | ||
| ac_key, | ||
| aux_ac_keys=[], | ||
| domain="", |
There was a problem hiding this comment.
low (simplification): Every caller passes domain, and stems are only ever registered as f"{domain}_{modality}". That makes the "" default and the stems.get(modality) fallback in _stem_history_len dead paths that silently produce history_len=1. Make domain required and index stems[f"{domain}_{modality}"] directly. Also build the error hint at 1434 from domain/modality instead of hardcoding model...state_ee_pose.history_len.
| seq_len = array.shape[0] | ||
| if seq_len >= history: | ||
| return array | ||
| padding = np.repeat(array[:1], history - seq_len, axis=0) |
There was a problem hiding this comment.
low (correctness): _front_pad_history repeats the oldest real frame, but MLPPolicyStem._drop_history replaces the past with the current frame. The two agree only while the window holds one real frame (idx < stride). Example with K=3, s=5, idx=7: the window is [2, 7], padded to [2, 2, 7], while a dropout sample is [7, 7, 7]. Either pad with array[-1:] so a short window is exactly a dropout sample, or keep this padding and fix the 'exactly a history-dropout sample' comments here, in hpt.py and in the stem docstring (and consider feeding proprio_history_mask to the stem).
| if horizon is not None: | ||
| end_idx = self._chunk_end_idx(idx, horizon, key_type) | ||
| read_interval = (idx, end_idx) | ||
| elif history is not None and int(history) > 1: |
There was a problem hiding this comment.
nit (simplification): The history condition is parsed three times (here, at 2293, and in _proprio_history), and stride at 2294 is bound only because the 2293 condition happens to match this elif. Normalize once at the top of the loop: history = int(spec.get("history") or 1); stride = int(spec.get("history_stride") or 1). Then branch on history > 1 in both places. _stride_history and _front_pad_history could also be merged into one helper.
| K = 1 path is bit-for-bit unchanged and the transform is idempotent. | ||
| """ | ||
|
|
||
| def __init__(self, keys: list[str], ndim: int = 1): |
There was a problem hiding this comment.
nit (dead-code): No caller sets ndim, and the transform only ever sees (D,) or (K, D) per sample. Dropping the parameter and the loop simplifies it to if value is not None and value.ndim == 2: batch[key] = value[-1].
…mbedding) The proprio keys the model consumes read K steps instead of one. Every change is gated on K > 1 (data) or history_len > 1 (model), and K = 1 / stride 1 is bit-for-bit today's pipeline. Data: Human.get_keymap gains proprio_history and history_stride; for K > 1 it marks the proprio entries with "history": K plus the stride, and ZarrDataset.__getitem__ reads the window idx - s*(K-1) ... idx in steps of s, clamped at the episode start and front-padded by repeating the first real frame, so the key is always (K, D) with the current frame last. A proprio_history_mask (1 = real, 0 = front padding) travels with it. The stride is why this ships on rather than at K = 1. Measured on a real flagship episode (30 fps, fold_clothes), the median wrist translation is 6.8 mm over 1 frame, 25.7 mm over 3, 42.9 mm over 5 and 80.8 mm over 10: an unstrided K = 3 window is 67 ms of past, close enough to a duplicate of the current pose to be mostly sensor noise, while the action chunk it conditions spans a full second. The flagship recipe therefore ships proprio_history: 3 / history_stride: 5 -- 0.33 s of past -- with the stem at history_len: 3. That changes the observation, so runs before and after this commit are not comparable, and history_dropout: 0.2 stops being inert: it is now a real hyperparameter, and the mitigation for the copycat/causal-confusion risk that proprio history classically brings. Transforms: the cartesian builders already treat a leading axis as batch; ActionChunkCoordinateFrameTransform / PoseCoordinateFrameTransform gain an opt-in target_history so a proprio target frame collapses to the CURRENT step (the action chunk must not move when history is added), and Reshape / QuaternionPoseToYPR tolerate the extra axis. observations.state.ee_pose is (K, 20). Eval: every human revert transform list starts with a SelectCurrentStep on its proprio key, so the val metrics, the cam-frame revert and the overlay video see the single current pose they always saw. No-op at K = 1 and idempotent. Norm stats are the baseline's: per-channel (D,) stats broadcast over (K, D), and both the training and the precompute stats pass force proprio_history=1 so K > 1 and K = 1 runs share one set. Model: MLPPolicyStem gains history_len / history_dropout. For history_len > 1 it owns a zero-init learned time embedding added after the projection, and HPT.stem_process skips the input-side sinusoid for such a stem (a constant [0, 1, 0, 1, ...] on normalized values is amplitude-1 noise on [-1, 1] data). history_dropout replaces the whole past by the current step per sample in training, exactly what an episode-start sample looks like after front padding. The K = 1 baseline stays one override away (proprio_history=1, history_stride=1, history_len=1) and the tests pin that it is bit-identical. Every other shipped config is untouched at K = 1, including the keypoint arm, whose wrist-frame proprio would need per-step framing before it can carry history. A compose test pins every train_*.yaml recipe's data K against its stem's history_len, because a recipe that inherits one model's data config and overrides the model is exactly how the two drift apart, and nothing else composes both halves. _stem_history_len takes the domain rather than matching a stem name by suffix: a cotrain run registers one state_ee_pose stem per domain, and a suffix match returns whichever comes first, so the human side was checked against eva's history_len. Latent while everything shipped K = 1; a blocker now. Co-Authored-By: Claude Fable 5.1 <[email protected]> Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01VBmhpVy7ZcMtsBJWh38cJp
51525b5 to
1e9470e
Compare
e9ac33c to
19e047a
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
|
Adds proprio history: the proprio keys the model consumes can read K past steps, spaced
history_strideframes apart, one token each with a learned time embedding. The flagship HPT recipe turns it on at K = 3, stride 5.Behaviour changes
history_len> 1 (model), and the tests pin that the K = 1 sample is bit-for-bit today's.data/mecka_fold_flagship_opsplit_hpt_6d.yamlsetsproprio_history: 3,history_stride: 5, andmodel/hpt_bc_mecka_6d_300M.yamlsets the humanstate_ee_posestem tohistory_len: 3,history_dropout: 0.2.train_zarr_mecka_flagship_6d_hptcomposes both.history_dropout: 0.2is now live. It was inert at K = 1. It is the mitigation for the copycat / causal-confusion risk that proprio history brings.The K = 1 baseline is one override away:
Why stride 5
On a real flagship episode (30 fps, fold_clothes), the median wrist translation is 6.8 mm over 1 frame, 25.7 mm over 3, 42.9 mm over 5 and 80.8 mm over 10. An unstrided K = 3 window covers 67 ms and is close to a duplicate of the current pose, mostly sensor noise, while the action chunk it conditions spans a full second. K = 3 at stride 5 is 0.33 s of past.
What changed
Human.get_keymapgainsproprio_historyandhistory_stride. For K > 1 it marks the proprio entries with"history": Kand the stride.ZarrDataset.__getitem__readsidx - s*(K-1) ... idxin steps of s, clamped at the episode start and front-padded by repeating the first real frame, so the key is always (K, D) with the current frame last. Aproprio_history_mask(1 = real, 0 = padding) travels with it; HPT drops it.ActionChunkCoordinateFrameTransform/PoseCoordinateFrameTransformgain an opt-intarget_history, so a proprio target frame collapses to the current step and the action chunk doesn't move when history is added.ReshapeandQuaternionPoseToYPRtolerate the extra axis.observations.state.ee_poseis (K, 20).SelectCurrentStepon its proprio key, so val metrics, the cam-frame revert and the overlay video see the single current pose they always saw. It is a no-op at K = 1 and idempotent.trainHydra.pyandprecompute_norm_stats.pyboth forceproprio_history=1for the stats pass, so K > 1 and K = 1 runs share one set.MLPPolicyStemgainshistory_len/history_dropout. Forhistory_len> 1 it owns a zero-init learned time embedding added after the projection, andHPT.stem_processskips the input-side sinusoid for that stem (a constant[0, 1, 0, 1, ...]on normalized values is amplitude-1 noise on [-1, 1] data).history_dropoutreplaces the whole past by the current step per sample in training, which is what an episode-start sample looks like after front padding.history_len._stem_history_lenlooks the stem up by domain rather than by name suffix. A cotrain run registers onestate_ee_posestem per domain, and a suffix match would have checked the human side against eva'shistory_len.Tests
New
tests/unit/test_proprio_history.py: the strided window and front padding, K = 1 bit-for-bit, the wrist-frame transform chain at (K, 20), norm broadcast and bounds check, the stem's time embedding and dropout,stem_process, the per-domain lookup, the eval path's blindness to history, the flagship's K = 3 / stride 5, and a compose test that pins everytrain_*.yamlrecipe's data K against its stem'shistory_len. That compose test exists because a recipe that inherits one model's data config and overrides the model is exactly how the two drift apart.🤖 Generated with Claude Code
https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw