Skip to content

Make zarr time axis dense - #291

Merged
sfmig merged 19 commits into
mainfrom
smg/make-zarr-time-dense
Sep 18, 2026
Merged

sfmig merged 19 commits into
mainfrom
smg/make-zarr-time-dense

Conversation

@sfmig

@sfmig sfmig commented Sep 16, 2026 •

Copy link
Copy Markdown
Collaborator

Description

What is this PR?

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?
Currently, the zarr store that collects the crab tracks across the full set of videos has a sparse time axis. This means that for a given clip, the time dimension has coordinates only for the frames with data. This is because the VIA tracks file only records detections, and therefore only logs frames with data.

When concatenating the data across clips of the same video, the time dimension becomes less sparse (as it will become the union of all 'coordinates' per clip), but there is no guarantee that it will be dense for any video.

Importantly, because the data per clip was loaded with use_frame_numbers_from_file=False, the frame numbers are mapped to ordinal numbers 0,1,2,... If there are frames with no detections in the clip, this can be confusing and lead to error (e.g. this would make the current definition of the escape_state array wrong).

What does this PR do?
It makes the time axis on the zarr dataset dense. Specificallu:

  • It loads the clip tracked data with use_frame_numbers_from_file=True, to retain the frame numbers from the VIA tracks file (which for the crabs case, are 0-based clip indices)
    • After movement PR 1103, using the default use_frame_numbers_from_file=False would give the same result in this case.
  • It gets the frame range from the clip metadata and reindexes the dataset.
    • We first ensure there are no frames in the dataset outside the range specified in the metadata, since in that case the reindexing operation would drop those values silently.

It also updates the script to generate output videos, so that the ds loaded from an input VIA tracks file has a dense axis that matches the frame in the video (it includes a check like the above to avoid silently dropping data out of range).

References

  • movement PR 1103
  • The plan provided to Claude to develop this feature is included in this PR's history here

How has this PR been tested?

Past and added tests pass locally and in CI

Does this PR require an update to the documentation?

It includes an update to the guide to generate a zarr dataset from tracks (guides/CreateZarrDatasetForTracks.md)

Checklist:

  • The code has been tested locally
  • Tests have been added to cover all new functionality
  • The documentation has been updated to reflect any changes
  • The code has been formatted with pre-commit

@sfmig sfmig changed the title Review proposal from claude Make zarr time axis dense Sep 16, 2026
sfmig and others added 6 commits September 16, 2026 13:51
Load VIA tracks files with `use_frame_numbers_from_file=True` so `time`
holds the clip's real frame numbers, then reindex onto the clip's full
frame range (from the metadata csv) so the axis is dense: a frame with no
tracked boxes is a NaN row rather than an absent label.

`escape_state` is now sized by the clip rather than by however many rows
survived, so its onset lands on the right frame.

Adds `_validate_frames_in_clip_range`, because `reindex` silently drops
out-of-range labels: a file tracked on the full video, or paired with the
wrong metadata row, would otherwise yield a mostly-NaN dataset with no
error. Also guards the escape frame, which previously could slice nothing
and leave `escape_state` all zeros.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The existing fixtures are the bug's best witness (a 101-frame clip whose
VIA tracks file holds boxes for 7 frames, with the escape at clip frame
51), so they are kept as they are and only asserted against harder.

Adds coverage for a contiguous tracks file, and for both new guards. The
factory grows an optional `frame_indices` argument so the guard tests can
write frames outside the clip's range; its default is unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
States that `time` is the clip's 0-based frame index and spans the whole
clip, gives the slice recipe for a single clip's frame range, says why
`dropna(dim="time", how="all")` is not that recipe, and warns that stores
built before this change mean something else.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment thread crabs/zarr/create_dataset.py Outdated
Comment thread crabs/zarr/create_dataset.py Outdated
Comment thread crabs/zarr/create_dataset.py Outdated
Comment thread crabs/zarr/create_dataset.py Outdated
Comment thread crabs/zarr/create_dataset.py Outdated
Comment thread scripts/visualization/output_video.py Outdated
Comment thread scripts/visualization/output_video.py Outdated
Comment thread scripts/visualization/output_video.py Outdated
Comment thread crabs/zarr/create_dataset.py
Comment on lines +31 to +33
By default the file holds boxes for a sparse set of frames of a
101-frame clip, which is the case the dense `time` axis is about.
Pass `frame_indices` to write boxes for a different set of frames.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
By default the file holds boxes for a sparse set of frames of a
101-frame clip, which is the case the dense `time` axis is about.
Pass `frame_indices` to write boxes for a different set of frames.
By default the file holds boxes for a sparse set of frames within a
101-frame clip. Pass `frame_indices` to write boxes for a different
set of frames.

sfmig and others added 4 commits September 16, 2026 17:42
Co-authored-by: sfmig <33267254+sfmig@users.noreply.github.com>
Signed-off-by: sfmig <33267254+sfmig@users.noreply.github.com>
The batched review suggestions were applied through the GitHub UI, so
they never went through pre-commit locally and reintroduced long
docstring, comment and f-string lines. E501 has no autofix, so
pre-commit.ci could not clean them up either.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@sfmig
sfmig marked this pull request as ready for review September 17, 2026 14:22
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 50.89%. Comparing base (77ae44d) to head (d8289a9).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #291      +/-   ##
==========================================
+ Coverage   50.53%   50.89%   +0.35%     
==========================================
  Files          26       26              
  Lines        1670     1682      +12     
==========================================
+ Hits          844      856      +12     
  Misses        826      826              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sfmig
sfmig merged commit 99a2fcb into main Sep 18, 2026
7 checks passed
@sfmig sfmig mentioned this pull request Sep 23, 2026
5 of 7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants