Repository navigation
Conversation
e93fc54 to
d470b0b
Compare
3f17dfa to
c76a447
Compare
d470b0b to
75f90fe
Compare
c76a447 to
865520b
Compare
75f90fe to
07d7a99
Compare
9339bac to
cfeee2a
Compare
9e561ae to
b97f495
Compare
cfeee2a to
e3becc1
Compare
b97f495 to
61037b6
Compare
e3becc1 to
1fb4886
Compare
61037b6 to
97bea62
Compare
1fb4886 to
c27cd6d
Compare
97bea62 to
635b94a
Compare
635b94a to
7d794a9
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): 2 nit.
| transformed_key_name: str, | ||
| mode: Literal["xyzwxyz", "xyzypr", "xyz"] = "xyzwxyz", | ||
| target_history: bool = False, | ||
| per_step_target: bool = True, |
There was a problem hiding this comment.
nit (dead-code): test_per_step_target_is_the_default only reads the signature default through inspect and re-imports PoseCoordinateFrameTransform, which the module already imports at the top. Drop it. The rationale is now written out three times (this docstring, the human.py comment block, and the test docstrings), and the human.py copy carries the 2026-09-17 out-of-range counts. Keep it once, here, without the measurement.
| ) | ||
| return batch | ||
| batch[self.transformed_key_name] = self._one_step( | ||
| batch[self.target_world], pose_world |
There was a problem hiding this comment.
nit (consistency): Pass the already-bound target_world here, as the per-step branch does, instead of re-reading batch[self.target_world].
PoseCoordinateFrameTransform gains per_step_target: with target_history K, step k's keypoints are expressed in step k's OWN wrist frame instead of the current step's. Re-expressing older steps in the current frame smears wrist motion into the articulation channels, and the palm-rigid MANO knuckles (1/5/13/17) have a very tight legitimate range, so they land far outside their per-cell bounds and worsen with age: measured 2026-09-17, 10.3 / 6.9 / 3.3 out-of-range cells per sample at history steps 0/1/2 against 0.0 at the current step. That cost 22,500 loader retries on one run and starved rank 0 into an NCCL watchdog kill. Per-step framing keeps every step a genuine state -- keypoint 0 is 0 at every step, so the norm stats and the bounds check describe all of them. The ACTION chunk keeps the current-step frame: its leading axis is the forward horizon, not history. It is the default, and it is inert unless the target itself was read with history and the pose carries the matching axis -- a keymap without proprio_history never emits that shape, so no existing call site changes. For anything that does, per-step is not a tuning choice: under the shipped K = 3 / stride 5 the oldest step is 10 frames back, where the measured wrist translation is 80.8 mm rather than the ~17 mm that already pushed 67 of 126 channels out of band. The flag remains as an escape hatch. Both halves are tested: the origin property at K=4, and bit-identical output at K=1 either way. Nothing shipped exercises this yet: no data config pairs proprio_history with a keypoint keymap, and the flagship keypoint configs are pinned at K = 1 because they sit below this commit. It is enablement for the keypoint history arm, not a fix to a live run. The per-step loop is also a Python loop over K (measured 59.5 -> 165.4 us per call at K=3, twice per sample); worth vectorising if that arm is ever configured, not before. CartesianYPRToRot6D also picks up the note about applying it to the proprio ee_pose, which the embodiment comment used to carry. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01VBmhpVy7ZcMtsBJWh38cJp
7d794a9 to
d8fc6e7
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
|

PoseCoordinateFrameTransform gains per_step_target: with target_history K,
step k's keypoints are expressed in step k's OWN wrist frame instead of the
current step's. Re-expressing older steps in the current frame smears wrist
motion into the articulation channels, and the palm-rigid MANO knuckles
(1/5/13/17) have a very tight legitimate range, so they land far outside their
per-cell bounds and worsen with age: measured 2026-09-17, 10.3 / 6.9 / 3.3
out-of-range cells per sample at history steps 0/1/2 against 0.0 at the
current step. That cost 22,500 loader retries on one run and starved rank 0
into an NCCL watchdog kill.
Per-step framing keeps every step a genuine state -- keypoint 0 is 0 at every
step, so the norm stats and the bounds check describe all of them. The ACTION
chunk keeps the current-step frame: its leading axis is the forward horizon,
not history.
Off by default, and the per-step path additionally requires a leading axis on
the target, which a keymap without proprio_history never emits -- so no
existing call site changes. Both halves are now tested: the origin property at
K=4, and bit-identical output at K=1 with the flag on and off.
The human keypoint eef-frame keymap turns it on. CartesianYPRToRot6D also
picks up the note about applying it to the proprio ee_pose, which the
embodiment comment used to carry.
🤖 Generated with Claude Code
https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw