Repository navigation
Conversation
5418cd4 to
8f17980
Compare
ee26a86 to
616528d
Compare
424caac to
7b68092
Compare
e05524d to
e57b442
Compare
e57b442 to
a023115
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 medium, 1 low, 3 nit.
| if "rot6d_passthrough" in state: | ||
| self.rot6d_passthrough = bool(state["rot6d_passthrough"]) | ||
| elif state.get("norm_stats"): | ||
| self.rot6d_passthrough = False |
There was a problem hiding this comment.
medium (backward-compat): This fallback only applies to checkpoints loaded through ModelWrapper.from_state. trainHydra always builds fresh norm stats (rot6d_passthrough=True) and then loads weights through trainer.fit(ckpt_path=...) or load_checkpoint_weights. Nothing restores the checkpoint's saved norm state. So resuming, requeuing or mode=eval on a pre-#657 checkpoint (e.g. Eva cartesian_6d) silently feeds raw rot6d to a model trained on normalized rot6d, with no error. When ckpt_path is set, read the checkpoint's norm_stats_state and carry its rot6d_passthrough (False when absent), or fail on a mismatch. Also copy the flag in set_norm_stats_from.
| if cfg.data.train_datasets[dataset_name] is None: | ||
| continue | ||
| print(f"[precompute] dataset={dataset_name}: instantiating (syncs S3) ...") | ||
| train_datasets[dataset_name] = hydra.utils.instantiate( |
There was a problem hiding this comment.
low (duplication): This replicates trainHydra's norm loop by copying it (keymap norm_mode rewrite, episode fingerprints, cache_inputs/norm_cache_key/write_cached, private _norm_run_metadata), and it has already diverged: it uses hydra.utils.instantiate rather than _instantiate_dataset, and it skips the norm_stats.norm_stats.get(emb) guard before the cache write. Since the whole point is producing the same cache key trainHydra would, consider extracting a shared compute_or_load_norm_stats(cfg, train_datasets, *, max_samples=None, write_cache=True) used by both.
|
|
||
| Usage (CPU node, repo root, emimic venv): | ||
| python egomimic/scripts/precompute_norm_stats.py \\ | ||
| --data mecka_all_6d --model pi0.5_bc_mecka_6d \\ |
There was a problem hiding this comment.
nit (stale-content): --data mecka_all_6d --model pi0.5_bc_mecka_6d don't exist under hydra_configs at this head. Use shipped ones (e.g. --data mecka --model pi0.5_bc_mecka) and drop the 'emimic venv' reference.
| # chunk is exactly the identity pose, and in a wrist-frame keypoint proprio the | ||
| # wrist keypoint sits at its own frame origin while kp9 defines the forward | ||
| # axis, so kp0 and kp9's off-axis components are zero at EVERY timestep (28 | ||
| # action cells and 10 proprio channels on a real 144-D episode). A bare |
There was a problem hiding this comment.
nit (comments): This comment carries measurements ('28 action cells and 10 proprio channels on a real 144-D episode') and a comparison with Diffusion Policy/robomimic/GR00T that belong in the PR description. Two lines are enough: the threshold exists for structural-zero channels (wrist-frame t=0, kp0/kp9), and such channels normalize to 0 and unnormalize to their center.
| # cell, which keeps resolution on the near-future steps of a relative-frame | ||
| # chunk (TRI's LBM makes the same choice for the same reason). Turn it on | ||
| # for checkpoint parity with an openpi-shaped recipe; the two have not been | ||
| # compared on a real run here. |
There was a problem hiding this comment.
nit (comments): Keep one line on what pool_horizon toggles (stats per channel over the whole chunk vs. per (t, channel) cell). 'The two have not been compared on a real run here' and the project list are experiment notes for the PR description.
Wrist-relative actions made a latent division safe-looking: the t=0 cell of a wrist-frame chunk is exactly the identity pose, so its quantile range is 0 and the historical `(q99 - q1) + 1e-6` denominator scaled any off-convention value by 1e6. A channel whose range is below NORM_MIN_RANGE (1e-4) is now treated as constant -- it normalizes to 0 and unnormalizes to its centre rather than dividing by epsilon. Diffusion Policy / robomimic ignore ranges under 1e-4 and GR00T maps min == max to 0; this is the same rule, applied in the shared _apply_norm_one / _apply_unnorm_one so the dataset and the tests agree. norm_stats.pool_horizon (default false, so nothing changes unless asked) computes one set of stats per channel over every timestep of the chunk, the way openpi does, instead of one per (timestep, channel) cell. The pooled stats are tiled back to the (T, D) shape, so normalize, the bounds check and checkpoints see the same layout either way. It is part of the norm-stat cache key, so flipping it cannot silently reuse stats computed the other way. precompute_norm_stats.py computes and caches the stats for a recipe outside a training run, keyed by the same file + content hash. Which of the two is better is unsettled: the literature is split (LBM per-timestep with a stated rationale, openpi / RDT / GR00T / Diffusion Policy / UMI / lerobot / Octo pooled) and nothing here has compared them on a real recipe, so the default stays the inherited per-timestep one and the config comment says what would make it worth flipping. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01VBmhpVy7ZcMtsBJWh38cJp
A 6D rotation is two columns of a rotation matrix: already bounded in [-1, 1], and per-channel centre-and-scale pulls the pair off the manifold without buying any resolution. The two projects that anchor an action chunk at near-identity the way this repo does -- TRI's LBM and UMI -- both exclude rotation from normalization for exactly that reason (LBM: "We avoid this normalization procedure for the 6D rotation in the poses to avoid corrupting the rotation matrix"; UMI sends its rotation channels through an identity normalizer). Per timestep AND normalized was the one combination with no precedent, and it is what manufactures the t=0 zero-range rotation cells NORM_MIN_RANGE exists to catch -- this removes them at the source. rot6d_channels() reads the indices off the existing layout tables, so the rule follows the layout rather than a second hardcoded list: the cartesian 18/20 widths and the 144-D keypoint layout, action and proprio alike. The ypr widths keep the old behaviour; an Euler angle is not a matrix column and those modes are legacy-only now. _apply_norm_one / _apply_unnorm_one take the channels and pass them through untouched, so the dataset path, the evaluator revert and the rollout revert all follow from one place, and the degenerate-channel guard is left to do what it is actually good at: the structural kp0 / kp9 position zeros. Norm-stat files stay valid -- what changes is how they are applied -- but a run before this commit and a run after it are not comparable, and the loss is now weighted differently between translation (unit scale) and rotation (raw). Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01VBmhpVy7ZcMtsBJWh38cJp
MultiDataset.to_state now records rot6d_passthrough. A checkpoint state that has stats but no flag predates the passthrough and normalized the rot6d channels, so from_state turns it off for those; fresh stats (empty state) keep it on. Without this, eval/rescore/requeue of such a checkpoint fed raw rot6d proprio and left predicted rotations in normalized space, silently. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01GqAcWA2Lk9zubUhKX5DW1v
a023115 to
9b9116e
Compare
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
|

Stops normalizing the rot6d rotation channels, guards zero-range channels against dividing by epsilon, and adds opt-in openpi-style horizon pooling for norm stats.
Behaviour changes
NORM_MIN_RANGE(1e-4) are treated as constant. They normalize to 0 and unnormalize to their centre instead of going through the old(q99 - q1) + 1e-6denominator, which scaled any off-convention value by 1e6.norm_stats.pool_horizonis off by default, so it changes nothing unless set.rot6d passthrough
rot6d_channels()inpose_utils.pyreads the indices off the existing layout tables._apply_norm_one/_apply_unnorm_onepass those channels through, so the dataset path, the evaluator revert and the rollout revert all follow from one place.Degenerate-channel guard
_apply_norm_one/_apply_unnorm_oneinaction_utils.py, so the dataset and the tests agree.Horizon pooling
norm_stats.pool_horizon: truecomputes one set of stats per channel over every timestep of the chunk, as openpi does, instead of one per (timestep, channel) cell.pool_horizonis part of the norm-stat cache key, so flipping it can't silently reuse stats computed the other way.Precompute script
egomimic/scripts/precompute_norm_stats.pycomputes and caches a recipe's stats on a CPU node outside a training run. It writes an explicitnorm_stats.jsonand fills the content-keyed cache under the same key trainHydra uses, so a plain training run on the same episodes and recipe hits the cache.Tests
tests/unit/test_eva_wristframe_norm_pool.py: pooled stats tiling,pool_horizonin the cache key, the constant-channel guard, rot6d passthrough per key and width, and no passthrough outside the 6D layouts.tests/unit/test_wrist6d_roundtrip.pypasses the rot6d channels through its normalize / unnormalize round trips.🤖 Generated with Claude Code
https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw