Repository navigation
Conversation
732de9b to
ec36005
Compare
9d1dd19 to
f01abdc
Compare
ec36005 to
183054e
Compare
f01abdc to
effdec5
Compare
183054e to
796c29c
Compare
2a00698 to
ee8dfd2
Compare
b91c1b3 to
592fa6f
Compare
ee8dfd2 to
76b71f7
Compare
592fa6f to
64303ff
Compare
76b71f7 to
119657d
Compare
64303ff to
e306f04
Compare
119657d to
bbbca3e
Compare
| return np.concatenate([array, np.repeat(array[-1:], missing, axis=0)], axis=0) | ||
|
|
||
| def _pad_sequences(self, data, horizon: int | None) -> dict: | ||
| """``_pad_to_horizon`` over a read result, in place. Keys are zarr keys.""" |
There was a problem hiding this comment.
nit (dead-code): ZarrDataset no longer uses _pad_sequences. Its only caller is zarr_dataset_action_expert.py:121, which pads a single key. Call self._pad_to_horizon(raw[zarr_key], horizon) there and delete _pad_sequences.
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.
Comments that could not be anchored to a diff line:
egomimic/rldb/zarr/zarr_dataset_multi.py:2321nit (duplication): _read_windows' (start, end-or-None) encoding is decoded twice (start + 1 if end is None else endhere and in the intervals loop). The stride is re-parsed withint(spec.get("history_stride", 1) or 1)in _read_windows and again below, though _proprio_history already returns (K, stride). Return half-open (start, stop) plus a single-frame flag from _read_windows, and reuse one (K, stride) helper, so getitem stops re-deriving them. Before this PR the stride was a local.
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 nit.
| max(0, idx - stride * (int(history) - 1)), | ||
| idx + 1, | ||
| start, end = windows[k][1] | ||
| stop = start + 1 if end is None else end |
There was a problem hiding this comment.
nit (duplication): _read_windows' (start, end-or-None) encoding is decoded twice (start + 1 if end is None else end here and in the intervals loop). The stride is re-parsed with int(spec.get("history_stride", 1) or 1) in _read_windows and again below, though _proprio_history already returns (K, stride). Return half-open (start, stop) plus a single-frame flag from _read_windows, and reuse one (K, stride) helper, so getitem stops re-deriving them. Before this PR the stride was a local.
ZarrDataset.__getitem__ issued a separate ZarrEpisode.read for every keymap entry, and several entries name the same zarr key with different windows: in the mecka keypoint keymap at proprio_history=4, left/right.obs_keypoints and the two wrist poses are each read once as a forward action chunk (i, i+H) and once as a backward proprio window (i-K+1, i+1). Ten reads over six distinct keys, each paying zarr's per-call metadata hop. It now makes one zarr call per key and slices each window out of what it read. Measured on a real mecka episode with that keymap, back to back in one process: 27.5 -> 15.6 ms a sample on the read path (1.76x), one read call instead of ten and six metadata hops instead of ten. read is ~70% of __getitem__, which is also what sets the epoch-boundary stall, since an epoch's first batch is one worker building the whole batch. Windows on one key merge into a single contiguous read only when every row between them lies in a chunk one of them already touches (ZarrEpisode.plan_reads); otherwise the groups are fetched together with one orthogonal index. Numeric keys are chunked 100 rows, so their windows always merge and the numbers above are this path. JPEG keys are one frame per chunk, where a plain union would read and ship every frame between two windows far apart -- the image-history window #678 adds on the front camera is exactly that case. Output is unchanged, except that an array shorter than a window now raises a ValueError naming the key; the old per-entry slice returned the short rows and padded them, and a lookup miss here would have escaped as StopIteration, which the DataLoader reads as the end of the epoch. The union is sliced first and padded second, so a chunk running off the end of the episode pads exactly as before, and each slice is copied because a slice of a shared span is a view where a per-entry read was not -- transforms are handed these arrays and may write into them. The test compares against a per-entry oracle over a RAMPED episode ([t, d] = t*1000 + d) rather than the shared synthetic fixture, which holds every numeric key constant in time and so cannot see a wrong frame offset at all. 5 keymaps x 9 indices x {ZarrDataset, ZarrAnnotationCutoffDataset}, plus the two paths nothing else in the repo covers: a key read as both a window and a single frame, and the JPEG-decode retry, which changes idx and so must recompute the windows and the read. The oracle compares values, so it would pass a loader that read the whole episode; separate tests count the ROWS read per key and pin plan_reads on both chunk sizes. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01M5dTGNydAZbRP1EsXgbp4W
e306f04 to
d4712b6
Compare
bbbca3e to
9ce191d
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
|

ZarrDataset.__getitem__now makes one zarr call per key, not one per keymap entry. Several entries name the same zarr key with different windows. In the mecka keypoint keymap atproprio_history=4,left/right.obs_keypointsand the two wrist poses are each read once as a forward action chunk(i, i+H)and once as a backward proprio window(i-K+1, i+1). That was ten reads over six distinct keys, each paying zarr's per-call metadata hop.Behaviour changes
Output is unchanged, with one exception: an array shorter than a window now raises a
ValueErrornaming the key. The old per-entry slice returned the short rows and padded them. A lookup miss here would otherwise escape asStopIteration, which the DataLoader reads as the end of the epoch.Speed
Measured on a real mecka episode with that keymap, back to back in one process:
Read is ~70% of
__getitem__. It also sets the epoch-boundary stall, since an epoch's first batch is one worker building the whole batch.How reads are planned
ZarrEpisode.plan_readsmerges the windows on one key into a single contiguous read only when every row between them lies in a chunk one of them already touches. Otherwise the groups are fetched together with one orthogonal index (ZarrEpisode.read_intervals)._pad_to_horizon), so a chunk running off the end of the episode pads exactly as before.Tests
tests/unit/test_loader_read_coalescing.py:[t, d] = t*1000 + d). The shared synthetic fixture holds every numeric key constant in time, so it can't see a wrong frame offset. The oracle covers 5 keymaps x 9 indices x {ZarrDataset,ZarrAnnotationCutoffDataset}.idxand must recompute the windows and the read.test_rows_read_are_the_union_of_the_windows,test_far_apart_jpeg_frames_are_two_rows_not_the_gap) and pinplan_readson both chunk sizes.test_an_array_shorter_than_its_window_fails_loudly.🤖 Generated with Claude Code
https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw