From e40048759ab4c7d51f4960d10e182bc2510014a0 Mon Sep 17 00:00:00 2001
From: sfmig <33267254+sfmig@users.noreply.github.com>
Date: Thu, 10 Sep 2026 17:38:53 +0100
Subject: [PATCH 01/19] Review proposal from claude

---
 .../proposal-detect-and-track-mask.md         | 544 ++++++++++++++++++
 1 file changed, 544 insertions(+)
 create mode 100644 claude-plans/proposal-detect-and-track-mask.md

diff --git a/claude-plans/proposal-detect-and-track-mask.md b/claude-plans/proposal-detect-and-track-mask.md
new file mode 100644
index 00000000..758772bf
--- /dev/null
+++ b/claude-plans/proposal-detect-and-track-mask.md
@@ -0,0 +1,544 @@
+# Proposal for a `detect-and-track-mask` entry point
+
+## Description
+
+This document proposes a **new CLI entry point, `detect-and-track-mask`** in the crabs backage.
+
+This entry point runs the existing
+`detect-and-track-video` pipeline unchanged, then prompts SAM2 with the **tracked** boxes and
+writes one ID-encoded mask per frame, where each mask carries its **track ID**. 
+
+
+> [!NOTE]
+> Nomenclature clarifications
+> * **Prompt** — a hint telling SAM2 *which* object to segment. Here, one bounding box in
+> `[x1, y1, x2, y2]` pixel coordinates per tracked crab.
+> * **Label image** — a 2-D integer array where `0` is background and every other value identifies
+> one object. This is the array `skimage.measure.regionprops` takes.
+
+
+## References
+It is a scoped-down
+first slice of Phase 3 of
+[`this-repo-collects-tools-pure-kernighan.md`](this-repo-collects-tools-pure-kernighan.md).
+
+---
+
+## Key aspects of suggested implementation
+
+
+
+### 1. A subclass, so the diff is one new file
+
+* `Tracking.detect_and_track_video` already produces exactly what SAM2 needs — tracked boxes and
+IDs per frame.
+* It already re-loops over the video twice (for `--save_video` and
+`--save_frames`). Masking is a third such loop.
+
+```mermaid
+flowchart TD
+    subgraph existing["detect-and-track-video (unchanged)"]
+        A["Tracking.detect_and_track_video()"] --> B["core_detection_and_tracking()<br/>loop 1: detect + SORT"]
+        B --> C["write_tracked_detections_to_csv<br/>&lt;video&gt;_tracks.csv"]
+        B --> D["generate_tracked_video<br/>loop 2 (optional)"]
+        B --> E["write_all_video_frames_as_images<br/>loop 3 (optional)"]
+    end
+    subgraph new["✨ detect-and-track-mask"]
+        F["TrackingAndMasking(Tracking)"] -.->|inherits, calls| A
+        F --> G["✨ generate_masks()<br/>loop 4: SAM2 per frame"]
+        G --> H["✨ &lt;video&gt;_masks.zarr"]
+    end
+    style F fill:#e0e7ff,stroke:#4338ca,color:#1e1b4b
+    style G fill:#e0e7ff,stroke:#4338ca,color:#1e1b4b
+    style H fill:#e0e7ff,stroke:#4338ca,color:#1e1b4b
+```
+
+* The **entire feature is one new module**, `crabs/tracker/track_and_mask_video.py`. 
+
+* The only change
+to existing Python is:
+    - one added line, so `generate_masks`in `crabs/tracker/track_and_mask_video.py` can see the boxes:
+
+    ```diff
+    --- a/crabs/tracker/track_video.py
+    +++ b/crabs/tracker/track_video.py
+    @@ def detect_and_track_video(self) -> None:
+        # Run detection and tracking over all frames in video
+        tracked_bboxes_dict = self.core_detection_and_tracking()
+    +   self.tracked_bboxes_dict = tracked_bboxes_dict
+    ```
+* `prep_outputs` in `generate_masks` needs no change: 
+    - `generate_masks`  can derive its output path from `self.tracking_output_dir` and `self.input_video_file_root`, in the same way as it does already.
+
+* The masks are
+the third optional artefact in the same directory as the video `--save_video` writes and the raw PNGs `--save_frames` writes.
+    ```
+    tracking_output_<timestamp>/
+    ├── <video>_tracks.csv
+    ├── <video>_tracks.mp4          # --save_video
+    ├── <video>_frames/             # --save_frames
+    └── <video>_masks.zarr          # ✨ always, for this entry point
+    ```
+
+### 2. Output format: an ID-encoded zarr `(T, H, W)` int32, labelled by track ID
+
+* This is the format `skimage.measure.regionprops` consumes directly, with no decoding step:
+
+    ```python
+    import zarr
+    from skimage.measure import regionprops
+
+    masks = zarr.open("<video>_masks.zarr", mode="r")
+    for prop in regionprops(masks[frame_idx]):
+        prop.label              # == the SORT track ID from <video>_tracks.csv
+        prop.orientation        # ... later
+    ```
+
+* **"masks inherit their ID from the tracked boxes"** falls out for free because `regionprops` returns one region per distinct non-zero value and exposes it as `prop.label`, so no sidecar mapping file.
+
+* It also matches what already exists in the repo:
+[`generate_masks_from_bboxes.py`](../scripts/generate_masks_from_bboxes.py) writes the same
+`(N, H, W)` shape with `chunks=(1, H, W)`, and
+[`notebook_visualise_masks_from_zarr.py`](../notebooks/notebook_visualise_masks_from_zarr.py)
+already loads that shape into napari.
+
+> [!WARNING]
+> **Do not copy that script's dtype.** `create_mask_zarr` opens the store as `dtype="bool"`
+> ([:120](../scripts/generate_masks_from_bboxes.py#L120)) while `predict_masks_across_images`
+> writes an `int16` ID-encoded array into it ([:191](../scripts/generate_masks_from_bboxes.py#L191)),
+> so every ID is silently coerced to `True` and the instance IDs are lost. This proposal uses
+> `int32` **and asserts the store dtype against the declared `mask_encoding` at creation**, so the
+> same bug cannot recur.
+
+* `int32` rather than `int16`/`uint16`. There **is** a hard ceiling on track IDs, contrary to what a
+never-reset monotonic counter suggests: 
+    * the detector caps detections at **100 per image**
+(`fasterrcnn_resnet50_fpn_v2` is constructed with no kwargs at
+[models.py:82](../crabs/detector/models.py#L82), so torchvision's `box_detections_per_img=100`
+applies). 
+    * `KalmanBoxTracker.count` increments only on an *unmatched* detection
+([sort.py:76-77](../crabs/tracker/sort.py#L76-L77)) — so `total IDs ≤ 100 × n_frames`. That bound
+still rules out `uint16` (65535) for anything past 655 frames, and the realistic count
+(≈ `n_crabs + n_ID_switches`) is a measurement nobody has taken yet. Since a sparse, blocky label
+image compresses well, `uint16`'s nominal 2× saving largely disappears in the store anyway.
+
+    So: 
+    *   `int32`, 
+    * **plus an assertion at the end of the run that `KalmanBoxTracker.count` fits the
+    dtype**, and 
+    * the final count recorded as `n_track_ids` in `.attrs` — which makes the dtype
+    choice measured rather than argued. See *Points to discuss* on file size.
+
+    (`KalmanBoxTracker.count` being a class attribute that is never reset
+    ([sort.py:39](../crabs/tracker/sort.py#L39)) does not affect this: the CLI runs one `Sort` per
+    process, so the counter starts at 0 on every run. It only bites if several trackers are
+    instantiated in one process, e.g. across tests.)
+
+* Mirroring the existing script's dict, this would be the Metadata that goes in `.attrs`: 
+
+```python
+{
+    "timestamp": ..., "sam2_model": ..., "device": ...,
+    "source_video": str(self.input_video_path),
+    "tracks_csv": str(self.csv_file_path),      # the file these IDs refer to
+    "n_frames": ..., "image_shape": [H, W],
+    "mask_encoding": "track_id",                 # asserted against the array dtype
+    "n_track_ids": ...,                          # KalmanBoxTracker.count — asserted to fit dtype
+    "background_label": 0,
+    "prompt_type": "bounding_box",
+    "prompt_source": "tracked_boxes",            # not raw detections — see §4
+    "overlap_policy": "higher_track_id_wins",
+    "multimask_output": False,
+}
+```
+
+### 3. Painting N boolean masks into one label image
+
+```mermaid
+flowchart LR
+    A["frame (H, W, 3) BGR"] --> B["cvtColor → RGB"]
+    B --> C["predictor.set_image<br/>encoder, once per frame"]
+    D["tracked_boxes (N, 4)<br/>ids (N,)"] --> E
+    C --> E["predictor.predict(box=chunk)<br/>decoder, per chunk of 32"]
+    E --> F["(N, H, W) bool"]
+    F --> G["✨ paint_masks_as_label_image<br/>ascending track ID"]
+    G --> H["(H, W) int32<br/>0 = background, else track ID"]
+    H --> I[("&lt;video&gt;_masks.zarr[t]")]
+    style G fill:#e0e7ff,stroke:#4338ca,color:#1e1b4b
+```
+
+```python
+def paint_masks_as_label_image(masks, track_ids, image_shape):
+    """Combine per-instance boolean masks into one int32 label image.
+
+    masks      : (N, H, W) bool
+    track_ids  : (N,) int, the SORT IDs from the tracks CSV
+    returns    : (H, W) int32, 0 = background
+
+    Where two masks overlap, the higher track ID wins. Masks are painted in
+    ascending track-ID order so the result does not depend on input order.
+    """
+    label_image = np.zeros(image_shape, dtype=np.int32)
+    for i in np.argsort(track_ids):
+        label_image[masks[i]] = track_ids[i]
+    return label_image
+```
+
+Overlap is real here — ~100 frequently touching crabs — and this policy loses the overlapping
+pixels of the lower-ID crab. It is deterministic and recorded in the attrs, but it is the main
+known limitation of the format; the alternative is in *Points to discuss*.
+
+### 4. Prompting with tracked boxes: what you get, and what it costs
+
+Per the request, the prompts are the **tracked** boxes. That is what makes the plumbing trivial —
+the mask rows and the CSV rows are the same rows, so there is no index bookkeeping at all, and no
+change to `sort.py`.
+
+Two consequences worth stating plainly, neither of which blocks this:
+
+- **SORT does not return the detector's box.** `Sort.update` emits `trk.get_state()` =
+  `convert_x_to_bbox(self.kf.x)` ([sort.py:204](../crabs/tracker/sort.py#L204)) — a Kalman-filtered
+  estimate in `(cx, cy, area, aspect_ratio)` space, lagged and with its aspect ratio pulled towards
+  the track's history. The masks are therefore segmented from an approximate box.
+- **Only tracked crabs get a mask.** Detections that SORT suppresses (below `min_hits`) produce no
+  row and no mask. With `min_hits: 1` in the current config this is nearly all of them.
+
+The compute difference is negligible either way: the expensive image encoder runs **once per
+frame** regardless, and prompts only touch the small mask decoder.
+
+### 5. Three SAM2 details that will silently degrade or crash this
+
+| Detail | Consequence | Handling |
+|---|---|---|
+| `cv2.VideoCapture.read()` returns **BGR**; `set_image` documents **RGB** (`sam2_image_predictor.py:86-98`) | Silently worse masks — no error, no warning. The existing script never hit this because it reads RGB PNGs via PIL. | `cv2.cvtColor(frame, cv2.COLOR_BGR2RGB)` |
+| `predict()` ends with `masks.squeeze(0)`: returns `(N, 1, H, W)` for N>1 but `(1, H, W)` for N==1 | Crash or wrong axis on any frame with exactly one tracked crab. The same latent bug is in [generate_masks_from_bboxes.py:171](../scripts/generate_masks_from_bboxes.py#L171). | `masks.reshape(len(boxes), *masks.shape[-2:])` |
+| Every prompt gets a **full-frame** mask: `_predict` upsamples to `self._orig_hw`, then `predict()` does `.float().cpu().numpy()` | At 1920×1080 that is 8.29 MB per prompt in float32 — **829 MB on CPU for a 100-crab frame**, and 100 prompts is the exact worst case, since the detector caps detections per image at 100 (§2) | Chunk the prompts (`max_prompts_per_batch`, default 32 → 265 MB peak) and paint each chunk into the label image before releasing it |
+
+Also: skip SAM2 entirely on frames with zero tracked boxes, rather than calling `set_image` and
+then `predict` with an empty array.
+
+### 6. Configuration and the `sam2` dependency
+
+The two knobs go in the **existing tracking config**, so the new entry point can reuse
+`tracking_parse_args` verbatim and adds **no CLI code**:
+
+```yaml
+# crabs/tracker/config/tracking_config.yaml   ✨ new block
+masks:
+  sam2_model_id: facebook/sam2.1-hiera-base-plus   # matches the existing script's default
+  max_prompts_per_batch: 32                        # see §5
+```
+
+Read it as `{**MASK_DEFAULTS, **self.config.get("masks", {})}`. This matters: the integration test
+fetches its `tracking_config.yaml` from the pooch/GIN registry
+([test_inference.py:45](../tests/test_integration/test_inference.py#L45)), and **that file has no
+`masks:` key**.
+
+On dependencies:
+
+- **`sam-2` is not on PyPI.** The only install route is the git URL used in
+  [generate_masks_from_bboxes.py:35](../scripts/generate_masks_from_bboxes.py#L35). A PEP 508
+  direct reference in `pyproject.toml` would make the sdist built by
+  [test_and_deploy.yml:59](../.github/workflows/test_and_deploy.yml#L59) unpublishable. It is
+  Apache-2.0, so there is no licence conflict — but it should be a **documented optional install
+  with a lazy import inside `generate_masks`**, raising an actionable error, not a declared
+  dependency. CI runs the unit suite on ubuntu and macOS without it.
+- **`zarr` is imported directly but not declared** — [`crabs/zarr/create_dataset.py:21`](../crabs/zarr/create_dataset.py#L21)
+  relies on it arriving transitively via `movement`. This proposal adds a second direct importer,
+  so it is worth declaring now (`zarr` 3.x is what `uv.lock` resolves).
+
+
+**Explicitly out of scope:** no ellipse fitting, no orientation, no angle in the CSV, no changes to
+the detector or to SORT. The masks are the deliverable; geometry comes later, offline, from the
+mask store.
+
+
+
+---
+
+## Detailed implementation
+
+### The 6 changes
+
+| # | Change | Signature / diff |
+|---|---|---|
+| 1 | **new** `crabs/tracker/track_and_mask_video.py` — the whole feature, ~150 lines | see below |
+| 2 | [`crabs/tracker/track_video.py`](../crabs/tracker/track_video.py) — expose the tracked boxes | `+ self.tracked_bboxes_dict = tracked_bboxes_dict` (one line, §1) |
+| 3 | [`crabs/tracker/config/tracking_config.yaml`](../crabs/tracker/config/tracking_config.yaml) | `masks:` block (§6) |
+| 4 | [`pyproject.toml`](../pyproject.toml) | `detect-and-track-mask = "crabs.tracker.track_and_mask_video:app_wrapper"`; declare `zarr` |
+| 5 | **new** `tests/test_unit/test_track_and_mask_video.py` | unit tests for the two pure helpers (no `sam2` needed) |
+| 6 | [`crabs/tracker/README.md`](../crabs/tracker/README.md) + [`guides/DetectAndTrackHPC.md`](../guides/DetectAndTrackHPC.md) | how to install `sam-2` and read the store back |
+
+<details>
+<summary><b>1. The new module in full outline</b></summary>
+
+Four module-level functions plus a four-method subclass. The first two functions are pure — no
+`sam2`, no torch, no I/O beyond zarr — which is what makes change #5 possible on CI.
+
+```python
+"""Detect, track and mask crabs in a video."""
+
+MASK_DEFAULTS = {
+    "sam2_model_id": "facebook/sam2.1-hiera-base-plus",
+    "max_prompts_per_batch": 32,
+}
+
+
+def paint_masks_as_label_image(masks, track_ids, image_shape) -> np.ndarray:
+    """(N, H, W) bool + (N,) ids -> (H, W) int32 label image.  [§3]"""
+
+
+def create_mask_zarr(path, n_frames, image_shape, metadata_dict) -> zarr.Array:
+    """Open an (T, H, W) int32 store, chunks=(1, H, W), and write .attrs.
+
+    Asserts the array dtype is integer, matching metadata["mask_encoding"].
+    """
+
+
+def load_sam2_predictor(model_id: str, device: str):
+    """Import sam2 lazily and return a SAM2ImagePredictor.  [§6]"""
+    try:
+        from sam2.sam2_image_predictor import SAM2ImagePredictor
+    except ImportError as e:
+        raise ImportError(
+            "detect-and-track-mask needs SAM2. Install it with:\n"
+            '  pip install "sam-2 @ git+https://github.com/facebookresearch/sam2.git"'
+        ) from e
+    return SAM2ImagePredictor.from_pretrained(model_id, device=device)
+
+
+def predict_masks_for_frame(predictor, frame_bgr, boxes, max_prompts_per_batch):
+    """(H, W, 3) BGR + (N, 4) boxes -> (N, H, W) bool.  [§5]"""
+    predictor.set_image(cv2.cvtColor(frame_bgr, cv2.COLOR_BGR2RGB))
+    out = []
+    FOR EACH chunk OF boxes, size max_prompts_per_batch:
+        masks, _iou, _low_res = predictor.predict(box=chunk, multimask_output=False)
+        out.append(masks.reshape(len(chunk), *masks.shape[-2:]).astype(bool))
+    RETURN np.concatenate(out)
+
+
+class TrackingAndMasking(Tracking):
+    """Extend Tracking with a SAM2 masking pass over the tracked boxes."""
+
+    def prep_mask_outputs(self): ...     # path + config defaults
+    def generate_masks(self): ...        # the loop, below
+
+
+def main(args):
+    inference = TrackingAndMasking(args)
+    inference.detect_and_track_video()   # inherited, unchanged
+    inference.generate_masks()
+
+
+def app_wrapper():
+    logging.getLogger().setLevel(logging.INFO)
+    torch.set_float32_matmul_precision("medium")
+    main(tracking_parse_args(sys.argv[1:]))   # reuses the existing parser verbatim
+```
+
+`generate_masks`, mirroring the structure of `write_all_video_frames_as_images`
+([io.py:205](../crabs/tracker/utils/io.py#L205)):
+
+```python
+def generate_masks(self):
+    predictor = load_sam2_predictor(cfg["sam2_model_id"], self.accelerator)
+    mask_zarr = create_mask_zarr(...)
+
+    input_video_object = open_video(self.input_video_path)
+    frame_idx = 0
+    WHILE input_video_object.isOpened():
+        ret, frame = input_video_object.read()
+        IF not ret:
+            parse_video_frame_reading_error_and_log(frame_idx, total_n_frames)
+            break
+
+        frame_data = self.tracked_bboxes_dict[frame_idx]
+        boxes, ids = frame_data["tracked_boxes"], frame_data["ids"]
+        IF len(boxes) > 0:                                    # [§5]
+            masks = predict_masks_for_frame(predictor, frame, boxes, chunk_size)
+            mask_zarr[frame_idx] = paint_masks_as_label_image(
+                masks, ids.astype(int), mask_zarr.shape[1:]
+            )
+        frame_idx += 1
+
+    input_video_object.release()
+```
+
+Frames with no tracked boxes are left at the store's `fill_value=0` — correct by construction, and
+one fewer branch.
+
+**Rejected alternative — masking inside `core_detection_and_tracking`.** It saves one video pass,
+but it puts SAM2 in the middle of the detection loop, changes an existing method, and makes the
+feature impossible to skip. A fourth pass matches the pattern the file already uses twice and
+keeps the diff to one added line in existing code.
+
+**Rejected alternative — a post-pass over an existing `_tracks.csv`.** More decoupled, and
+attractive later, but it means re-parsing VIA JSON and re-deriving the frame index from filenames
+when the same data is already in memory. It also could not reuse `Tracking` at all. Worth
+revisiting once the format is settled.
+</details>
+
+---
+
+## Files changed
+
+| File | Change |
+|---|---|
+| `crabs/tracker/track_and_mask_video.py` | **new** — the entire feature |
+| [`crabs/tracker/track_video.py`](../crabs/tracker/track_video.py) | one added line (store the tracked boxes on `self`) |
+| [`crabs/tracker/config/tracking_config.yaml`](../crabs/tracker/config/tracking_config.yaml) | `masks:` block |
+| [`pyproject.toml`](../pyproject.toml) | new `detect-and-track-mask` script; declare `zarr` |
+| `tests/test_unit/test_track_and_mask_video.py` | **new** |
+| [`crabs/tracker/README.md`](../crabs/tracker/README.md) | document the entry point, the store layout and how to read it back |
+| [`guides/DetectAndTrackHPC.md`](../guides/DetectAndTrackHPC.md) | installing `sam-2` on the cluster |
+
+Nothing else is touched. In particular: no change to `sort.py`, to `utils/io.py`, to the CSV
+format, to `evaluate_tracker.py`, or to the detector.
+
+---
+
+## Overview of tests to write
+
+**Pure unit — must pass with no `sam2` installed (this is the CI shape)**
+
+1. `paint_masks_as_label_image`: three non-overlapping masks with IDs `[7, 3, 12]` produce a label
+   image whose non-zero values are exactly `{3, 7, 12}` and whose regions are in the right places.
+2. Overlap policy: two overlapping masks with IDs `3` and `12` — the shared pixels are `12`, and
+   the result is identical when the inputs are passed in the opposite order (the `argsort` is doing
+   its job).
+3. Empty input `(0, H, W)` returns an all-zero image and does not raise.
+4. Round-trip through the consumer: feed the output of #1 to `skimage.measure.regionprops` and
+   assert `{p.label for p in props} == {3, 7, 12}`. This is the property the whole format exists
+   for, so it should be asserted rather than assumed.
+5. `create_mask_zarr`: the array dtype is integer, `fill_value` is 0, chunks are `(1, H, W)`, and
+   `.attrs["mask_encoding"] == "track_id"` — plus a test that the internal assertion **fires** if
+   the dtype and the declared encoding disagree (the guard against the `dtype="bool"` bug).
+6. `load_sam2_predictor` raises `ImportError` with the install command in the message when `sam2`
+   is absent (`monkeypatch` the import).
+7. The track-ID/dtype guard (§2): a track ID exceeding `np.iinfo(dtype).max` raises rather than
+   wrapping around silently. Cheap to test directly on the helper, and it is the assertion that
+   makes a later switch to `uint16` safe.
+
+**Existing tests that must stay green, unmodified**
+
+8. `pytest tests/test_unit` — in particular
+   [`test_tracking_io.py`](../tests/test_unit/test_tracking_io.py) (the CSV is untouched) and
+   [`test_track_video.py`](../tests/test_unit/test_track_video.py) (the one-line change to
+   `detect_and_track_video` must not alter behaviour).
+9. [`test_entry_points.py`](../tests/test_unit/test_entry_points.py) — extend with
+   `detect-and-track-mask` following the existing pattern.
+
+**Integration (slow, opt-in)**
+
+10. A `test_detect_and_track_mask` alongside
+   [`test_detect_and_track_video`](../tests/test_integration/test_inference.py), reusing the
+   `pooch_registry` fixture and the 3-frame clip, `@pytest.mark.skipif` on `sam2` being importable.
+   Assert the store exists, has shape `(3, H, W)`, and that the set of non-zero labels in each
+   frame equals the set of track IDs in that frame's rows of the CSV. That last assertion is the
+   real contract of this feature.
+
+---
+
+## Verifications for agent to run
+
+```bash
+# lint + full unit suite (no sam2 needed)
+pre-commit run --all-files
+pytest tests/test_unit
+
+# slow end-to-end CLI tests (pooch downloads test data on first run)
+pytest -m slow tests/test_integration/test_inference.py
+```
+
+Manual check on a real clip:
+
+```bash
+pip install "sam-2 @ git+https://github.com/facebookresearch/sam2.git"
+
+detect-and-track-mask \
+    --trained_model_path <ckpt> \
+    --video_path <clip.mp4> \
+    --config_file <tracking_config.yaml> \
+    --accelerator=gpu \
+    --output_dir_no_timestamp
+```
+
+then confirm the contract and eyeball the masks:
+
+```python
+import zarr, numpy as np
+from skimage.measure import regionprops
+
+masks = zarr.open("tracking_output/<clip>_masks.zarr", mode="r")
+print(masks.shape, masks.dtype, dict(masks.attrs))
+
+props = regionprops(masks[0])
+print(sorted(p.label for p in props))     # must match the track IDs in <clip>_tracks.csv frame 0
+print([p.area for p in props][:10])       # sanity: no zero-area or whole-frame regions
+```
+
+[`notebooks/notebook_visualise_masks_from_zarr.py`](../notebooks/notebook_visualise_masks_from_zarr.py)
+already loads this shape into napari; overlaying it on the frames written by `--save_frames` is the
+fastest qualitative check that the masks are on the crabs and not on the background.
+
+Also worth recording once, since it decides the format's viability: `du -sh` of the store for one
+real clip, and the wall-clock time of the masking pass versus the detection pass.
+
+---
+
+## Points to discuss
+
+1. **Is the overlap loss acceptable?** A single label image cannot represent two crabs occupying
+   the same pixel, and this scene has ~100 frequently touching individuals — so the lower-ID crab's
+   overlapping pixels are discarded (§3). Ellipse fits on a partly-eaten mask will be biased. The
+   alternative is storing **per-instance** masks (COCO RLE via `pycocotools`, already a dependency,
+   keyed by frame and track ID), which is lossless but is no longer directly `regionprops`-shaped —
+   it needs a `mask.decode()` first. My recommendation is to start with the label image, since it
+   is simpler and matches the existing repo convention, and measure how many pixels are actually
+   lost before deciding — that measurement is one extra line in `paint_masks_as_label_image` and
+   could be stored as a per-frame attr. Worth adding now?
+2. **Store size.** `(T, H, W)` int32 at 1920×1080 is 8.29 MB/frame uncompressed, so ~25 GB raw for
+   a 3000-frame clip. Label images are sparse and blocky so zarr's default compressor should reduce
+   that by a large factor, but I have not measured it and will not guess. If it turns out too big,
+   the levers are, in order: `uint16` (2×, at the cost of a 65535 track-ID ceiling — reachable in
+   principle, since the 100-detections-per-image cap only bounds IDs at `100 × n_frames`; see §2),
+   a stronger compressor, or the per-instance-RLE format from point 1 (which is far smaller because
+   it never stores background).
+3. **`sam2_model_id` default.** I have matched the existing script's `-base-plus`
+   ([generate_masks_from_bboxes.py:307](../scripts/generate_masks_from_bboxes.py#L307)) so the two
+   agree. `-tiny` / `-small` are considerably faster and may well be enough at this object size —
+   easy to compare once the entry point exists, since it is a config value.
+4. **Should the entry point re-run detection, or accept an existing tracking output?** As proposed
+   it re-runs everything, which is the simplest thing and matches the name. The cost is that
+   iterating on SAM2 settings means re-running the detector each time. A `--tracking_output_dir`
+   flag that skips straight to `generate_masks` would fix that and fits issue
+   [#249](https://github.com/SainsburyWellcomeCentre/crabs-exploration/issues/249)
+   ("Uncouple pipeline steps"), but it needs the tracks CSV parsed back in. Deliberately left out;
+   easy to add later without changing the format.
+5. **Not verified: the SAM2 *checkpoint* licence.** The `sam-2` package is Apache-2.0 (read from
+   the installed `dist-info`), and I assumed the `facebook/sam2.1-hiera-*` weights on Hugging Face
+   are too — but I did not open the model card.
+6. **⚠️ The detector is running at its detection cap.** `fasterrcnn_resnet50_fpn_v2` is constructed
+   with no kwargs ([models.py:82](../crabs/detector/models.py#L82)), so torchvision's default
+   `box_detections_per_img=100` applies — and this scene has **~100 crabs per frame**. Dense frames
+   are therefore plausibly being truncated to the top 100 by score, silently, before tracking and
+   before any masking. This does not change anything in this proposal — the masks follow whatever
+   the tracker emits — but it caps what the masks can ever cover, and it is invisible in the
+   outputs. Worth its own issue and a quick check: log
+   `max(len(boxes) for boxes in detections)` over a real clip and see how often it sits at exactly
+   100.
+7. **Two more things found while reading, both out of scope.** (a) `--max_frames_to_read` is parsed at
+   [track_video.py:440](../crabs/tracker/track_video.py#L440) and never used by anything — see PR
+   [#245](https://github.com/SainsburyWellcomeCentre/crabs-exploration/pull/245). It would make
+   iterating on this entry point much cheaper. (b) `write_tracked_detections_to_csv` zips the
+   tracked boxes against `detections_dict["scores"]`, which holds *all raw, unthresholded*
+   detections in detector order ([track_video.py:267](../crabs/tracker/track_video.py#L267),
+   [io.py:77-82](../crabs/tracker/utils/io.py#L77-L82)) — different length and different order, so
+   the CSV `confidence` column is effectively arbitrary. Neither affects the masks; both are worth
+   separate PRs.
+
+---
+
+## Feedback on the format of this proposal
+
+If any section is too long, too short, or in the wrong order for how you actually review, say so.

From 1a30d0bb621a4177bc30a95fedd3b7f79a512f6b Mon Sep 17 00:00:00 2001
From: sfmig <33267254+sfmig@users.noreply.github.com>
Date: Fri, 11 Sep 2026 14:46:58 +0100
Subject: [PATCH 02/19] Review output array shape

---
 .../proposal-detect-and-track-mask.md         | 700 ++++++++++++------
 1 file changed, 491 insertions(+), 209 deletions(-)

diff --git a/claude-plans/proposal-detect-and-track-mask.md b/claude-plans/proposal-detect-and-track-mask.md
index 758772bf..45c1d03d 100644
--- a/claude-plans/proposal-detect-and-track-mask.md
+++ b/claude-plans/proposal-detect-and-track-mask.md
@@ -6,15 +6,20 @@ This document proposes a **new CLI entry point, `detect-and-track-mask`** in the
 
 This entry point runs the existing
 `detect-and-track-video` pipeline unchanged, then prompts SAM2 with the **tracked** boxes and
-writes one ID-encoded mask per frame, where each mask carries its **track ID**. 
+writes one boolean mask per crab per frame, each stored in a plane indexed by that crab's
+**track ID**.
 
 
 > [!NOTE]
 > Nomenclature clarifications
 > * **Prompt** — a hint telling SAM2 *which* object to segment. Here, one bounding box in
 > `[x1, y1, x2, y2]` pixel coordinates per tracked crab.
+>
+> * **Instance plane** — a 2-D boolean array holding exactly one crab's mask. The store is a stack
+> of these, indexed by track ID. This is what is written to disk.
 > * **Label image** — a 2-D integer array where `0` is background and every other value identifies
-> one object. This is the array `skimage.measure.regionprops` takes.
+> one object. This is the array `skimage.measure.regionprops` takes. Here it is **derived on read**
+> from the instance planes (§3), never stored.
 
 
 ## References
@@ -53,10 +58,12 @@ flowchart TD
     style H fill:#e0e7ff,stroke:#4338ca,color:#1e1b4b
 ```
 
-* The **entire feature is one new module**, `crabs/tracker/track_and_mask_video.py`. 
+* The **entire feature is two new modules**: `crabs/tracker/track_and_mask_video.py` (the write
+path) and `crabs/tracker/utils/masks.py` (the read-side helper, §3). The second exists only
+because the overlap decision moved to read time; it imports neither `sam2` nor torch.
 
-* The only change
-to existing Python is:
+* There are **two changes to existing Python**, both in
+[`crabs/tracker/track_video.py`](../crabs/tracker/track_video.py):
     - one added line, so `generate_masks`in `crabs/tracker/track_and_mask_video.py` can see the boxes:
 
     ```diff
@@ -67,7 +74,11 @@ to existing Python is:
         tracked_bboxes_dict = self.core_detection_and_tracking()
     +   self.tracked_bboxes_dict = tracked_bboxes_dict
     ```
-* `prep_outputs` in `generate_masks` needs no change: 
+    - splitting `tracking_parse_args` into a parser builder and the call to `parse_args`, so the
+    new entry point can inherit the tracking arguments with `parents=[...]` (§5). Behaviour of
+    `detect-and-track-video` is unchanged.
+
+* `prep_outputs` in `generate_masks` needs no change:
     - `generate_masks`  can derive its output path from `self.tracking_output_dir` and `self.input_video_file_root`, in the same way as it does already.
 
 * The masks are
@@ -80,61 +91,70 @@ the third optional artefact in the same directory as the video `--save_video` wr
     └── <video>_masks.zarr          # ✨ always, for this entry point
     ```
 
-### 2. Output format: an ID-encoded zarr `(T, H, W)` int32, labelled by track ID
+### 2. Output format: a zarr `(T, M, H, W)` boolean array, one plane per track ID
+
+```
+<video>_masks.zarr/                  # zarr group, matching the existing store layout
+└── masks   (T, M, H, W) bool        # dims: image_id, id, img_h, img_w
+```
 
-* This is the format `skimage.measure.regionprops` consumes directly, with no decoding step:
+* Axis 1 is indexed by track ID, with `m = track_id - 1`. That mapping is exact, not a convention:
+`KalmanBoxTracker` numbers itself from a 0-based class counter
+([sort.py:76-77](../crabs/tracker/sort.py#L76-L77)) and `Sort.update` emits `trk.id + 1`
+([sort.py:210](../crabs/tracker/sort.py#L210)), so the IDs in `<video>_tracks.csv` are 1-based.
 
     ```python
     import zarr
-    from skimage.measure import regionprops
 
-    masks = zarr.open("<video>_masks.zarr", mode="r")
-    for prop in regionprops(masks[frame_idx]):
-        prop.label              # == the SORT track ID from <video>_tracks.csv
-        prop.orientation        # ... later
+    masks = zarr.open("<video>_masks.zarr", mode="r")["masks"]
+    masks[t, track_id - 1]     # (H, W) bool — one crab in one frame, SAM2's output verbatim
+    masks[t]                   # (M, H, W)   — every crab in frame t
+    masks[:, track_id - 1]     # (T, H, W)   — one crab's whole trajectory, in a single slice
     ```
 
-* **"masks inherit their ID from the tracked boxes"** falls out for free because `regionprops` returns one region per distinct non-zero value and exposes it as `prop.label`, so no sidecar mapping file.
+* **Why per-instance planes and not a single label image.** A label image cannot represent two
+crabs occupying the same pixel, and this scene has ~100 frequently touching individuals — so
+flattening would silently discard one crab's overlapping pixels. Here each crab owns its plane:
+overlap is representable, nothing is discarded, and there is **no overlap policy to choose**. The
+ellipse fits Phase 3 is aiming at are therefore not biased by neighbours.
 
-* It also matches what already exists in the repo:
-[`generate_masks_from_bboxes.py`](../scripts/generate_masks_from_bboxes.py) writes the same
-`(N, H, W)` shape with `chunks=(1, H, W)`, and
+* **It is the shape the existing consumer already builds by hand.**
 [`notebook_visualise_masks_from_zarr.py`](../notebooks/notebook_visualise_masks_from_zarr.py)
-already loads that shape into napari.
-
-> [!WARNING]
-> **Do not copy that script's dtype.** `create_mask_zarr` opens the store as `dtype="bool"`
-> ([:120](../scripts/generate_masks_from_bboxes.py#L120)) while `predict_masks_across_images`
-> writes an `int16` ID-encoded array into it ([:191](../scripts/generate_masks_from_bboxes.py#L191)),
-> so every ID is silently coerced to `True` and the instance IDs are lost. This proposal uses
-> `int32` **and asserts the store dtype against the declared `mask_encoding` at creation**, so the
-> same bug cannot recur.
-
-* `int32` rather than `int16`/`uint16`. There **is** a hard ceiling on track IDs, contrary to what a
-never-reset monotonic counter suggests: 
-    * the detector caps detections at **100 per image**
-(`fasterrcnn_resnet50_fpn_v2` is constructed with no kwargs at
-[models.py:82](../crabs/detector/models.py#L82), so torchvision's `box_detections_per_img=100`
-applies). 
-    * `KalmanBoxTracker.count` increments only on an *unmatched* detection
-([sort.py:76-77](../crabs/tracker/sort.py#L76-L77)) — so `total IDs ≤ 100 × n_frames`. That bound
-still rules out `uint16` (65535) for anything past 655 frames, and the realistic count
-(≈ `n_crabs + n_ID_switches`) is a measurement nobody has taken yet. Since a sparse, blocky label
-image compresses well, `uint16`'s nominal 2× saving largely disappears in the store anyway.
-
-    So: 
-    *   `int32`, 
-    * **plus an assertion at the end of the run that `KalmanBoxTracker.count` fits the
-    dtype**, and 
-    * the final count recorded as `n_track_ids` in `.attrs` — which makes the dtype
-    choice measured rather than argued. See *Points to discuss* on file size.
-
-    (`KalmanBoxTracker.count` being a class attribute that is never reset
-    ([sort.py:39](../crabs/tracker/sort.py#L39)) does not affect this: the CLI runs one `Sort` per
-    process, so the counter starts at 0 on every run. It only bites if several trackers are
-    instantiated in one process, e.g. across tests.)
-
-* Mirroring the existing script's dict, this would be the Metadata that goes in `.attrs`: 
+takes a `(T, H, W)` label image and one-hot expands it into exactly
+`(image_id, id, img_h, img_w)` boolean with a dask `map_blocks`
+([:142-174](../notebooks/notebook_visualise_masks_from_zarr.py#L142-L174)), under the comment
+*"CONS: can be slow to compute"*. Writing the planes directly deletes that expansion and its cost,
+and `sel(image_id=..., id=...)`
+([:181](../notebooks/notebook_visualise_masks_from_zarr.py#L181)) keeps working unchanged. The dim
+names above are taken from that notebook.
+
+* **`bool`, and the whole dtype question disappears.** Identity now lives in the *axis*, not in the
+pixel values, so there is nothing to encode and no `int16`/`int32`/`uint16` ceiling to argue about
+— the `100 × n_frames` bound on track IDs no longer constrains the format at all. This also
+disarms the trap in the existing script rather than merely guarding against it:
+
+> [!NOTE]
+> [`generate_masks_from_bboxes.py`](../scripts/generate_masks_from_bboxes.py) opens its store as
+> `dtype="bool"` ([:120](../scripts/generate_masks_from_bboxes.py#L120)) but writes an `int16`
+> ID-encoded array into it ([:191](../scripts/generate_masks_from_bboxes.py#L191)), so every ID is
+> silently coerced to `True` and the instance IDs are lost. Under this format `bool` is the
+> *correct* dtype, because no ID is ever stored in a pixel. The bug cannot be expressed.
+
+* **`M` is known before the masking loop starts**, so the array is never resized: §1 runs detection
+and tracking to completion first, so `M = max(track_id)` over `self.tracked_bboxes_dict`. Not
+every ID in `1..M` is necessarily emitted — a tracker suppressed below `min_hits` still burns a
+counter value — so a few planes stay empty for the whole video. Those are never written.
+
+* **Chunking, and the one real cost of this shape.** `chunks=(1, 1, H, W)` — one chunk per
+(frame, crab), the unit every access pattern above reads. A chunk is 2.07 MB raw at 1920×1080 and
+~0.17% non-zero, so it compresses to a few KB, and zarr does not write all-`False` chunks at all.
+The count is the problem: ~`n_frames × n_crabs` ≈ **300,000 chunks** for a 3000-frame clip, which
+means 300,000 files in a directory store — genuinely bad on the cluster's network filesystem. The
+mitigation is zarr 3's **sharding codec**, `shards=(1, 128, H, W)`, packing 128 planes per file and
+cutting that to `n_frames × ceil(M/128)`. The writer emits a whole frame at a time, so each shard
+is filled in one pass, and `zarr` is declared as `>=3` for it (§6). See *Points to discuss*.
+
+* Metadata in `.attrs` on the group:
 
 ```python
 {
@@ -142,17 +162,26 @@ image compresses well, `uint16`'s nominal 2× saving largely disappears in the s
     "source_video": str(self.input_video_path),
     "tracks_csv": str(self.csv_file_path),      # the file these IDs refer to
     "n_frames": ..., "image_shape": [H, W],
-    "mask_encoding": "track_id",                 # asserted against the array dtype
-    "n_track_ids": ...,                          # KalmanBoxTracker.count — asserted to fit dtype
-    "background_label": 0,
+    "dims": ["image_id", "id", "img_h", "img_w"],
+    "mask_encoding": "instance_planes",          # asserted against the array dtype (bool)
+    "track_id_offset": 1,                        # m = track_id - track_id_offset
+    "n_track_ids": ...,                          # M, the size of axis 1
+    "n_track_ids_emitted": ...,                  # planes non-empty in at least one frame
+    "kalman_box_tracker_count": ...,             # KalmanBoxTracker.count, for cross-checking
     "prompt_type": "bounding_box",
     "prompt_source": "tracked_boxes",            # not raw detections — see §4
-    "overlap_policy": "higher_track_id_wins",
     "multimask_output": False,
 }
 ```
 
-### 3. Painting N boolean masks into one label image
+There is deliberately **no `overlap_policy` and no `background_label`** key: this format has
+neither, which is the point of it.
+
+### 3. Writing planes, and rebuilding a label image on read
+
+SAM2 already returns per-instance masks, so the write path has **no combining step at all** — the
+masks go straight into their planes. The flattening that `regionprops` needs happens on the read
+side, where the caller chooses the policy and nothing on disk is affected.
 
 ```mermaid
 flowchart LR
@@ -161,32 +190,44 @@ flowchart LR
     D["tracked_boxes (N, 4)<br/>ids (N,)"] --> E
     C --> E["predictor.predict(box=chunk)<br/>decoder, per chunk of 32"]
     E --> F["(N, H, W) bool"]
-    F --> G["✨ paint_masks_as_label_image<br/>ascending track ID"]
-    G --> H["(H, W) int32<br/>0 = background, else track ID"]
-    H --> I[("&lt;video&gt;_masks.zarr[t]")]
-    style G fill:#e0e7ff,stroke:#4338ca,color:#1e1b4b
+    F -->|"✨ oindex[t, ids-1]"| I[("&lt;video&gt;_masks.zarr<br/>(T, M, H, W) bool")]
+    I -.->|"read side"| J["✨ to_label_image(store, t)"]
+    J -.-> K["(H, W) int32<br/>for regionprops / napari"]
+    style I fill:#e0e7ff,stroke:#4338ca,color:#1e1b4b
+    style J fill:#e0e7ff,stroke:#4338ca,color:#1e1b4b
 ```
 
+**Write side** — one orthogonal-index assignment per chunk of prompts, no helper needed:
+
 ```python
-def paint_masks_as_label_image(masks, track_ids, image_shape):
-    """Combine per-instance boolean masks into one int32 label image.
+mask_zarr.oindex[frame_idx, track_ids - 1] = masks   # (N, H, W) bool into N planes
+```
+
+`oindex` rather than plain `[...]` because zarr implements vectorised integer indexing through
+orthogonal indexing; `track_ids` are scattered along axis 1, not contiguous.
 
-    masks      : (N, H, W) bool
-    track_ids  : (N,) int, the SORT IDs from the tracks CSV
-    returns    : (H, W) int32, 0 = background
+**Read side** — one helper in a new `crabs/tracker/utils/masks.py`, pure (no `sam2`, no torch), so
+it is unit-testable on CI:
 
-    Where two masks overlap, the higher track ID wins. Masks are painted in
-    ascending track-ID order so the result does not depend on input order.
+```python
+def to_label_image(mask_planes, track_ids=None, policy="higher_track_id_wins"):
+    """(M, H, W) bool -> (H, W) int32 label image, for regionprops and napari.
+
+    This is where the overlap decision now lives: it is the caller's, per call,
+    and it never touches what is stored.
     """
-    label_image = np.zeros(image_shape, dtype=np.int32)
-    for i in np.argsort(track_ids):
-        label_image[masks[i]] = track_ids[i]
-    return label_image
 ```
 
-Overlap is real here — ~100 frequently touching crabs — and this policy loses the overlapping
-pixels of the lower-ID crab. It is deterministic and recorded in the attrs, but it is the main
-known limitation of the format; the alternative is in *Points to discuss*.
+`to_label_image` is the old `paint_masks_as_label_image` moved to the read side and given a
+`policy` argument. `ellipses_from_labels` in the notebook
+([:52](../notebooks/notebook_visualise_masks_from_zarr.py#L52)) consumes its output unchanged.
+
+**What this costs.** The store is no longer directly openable in napari — `viewer.add_labels` on a
+`(T, M, H, W)` boolean array gives a slider over crabs, not a frame view. Callers go through
+`to_label_image` instead, which is one line at the two call sites in the notebook
+([:219](../notebooks/notebook_visualise_masks_from_zarr.py#L219),
+[:226-229](../notebooks/notebook_visualise_masks_from_zarr.py#L226-L229)). That is the honest
+trade for losslessness; the notebook change is listed in *Files changed*.
 
 ### 4. Prompting with tracked boxes: what you get, and what it costs
 
@@ -206,47 +247,184 @@ Two consequences worth stating plainly, neither of which blocks this:
 The compute difference is negligible either way: the expensive image encoder runs **once per
 frame** regardless, and prompts only touch the small mask decoder.
 
-### 5. Three SAM2 details that will silently degrade or crash this
 
-| Detail | Consequence | Handling |
-|---|---|---|
-| `cv2.VideoCapture.read()` returns **BGR**; `set_image` documents **RGB** (`sam2_image_predictor.py:86-98`) | Silently worse masks — no error, no warning. The existing script never hit this because it reads RGB PNGs via PIL. | `cv2.cvtColor(frame, cv2.COLOR_BGR2RGB)` |
-| `predict()` ends with `masks.squeeze(0)`: returns `(N, 1, H, W)` for N>1 but `(1, H, W)` for N==1 | Crash or wrong axis on any frame with exactly one tracked crab. The same latent bug is in [generate_masks_from_bboxes.py:171](../scripts/generate_masks_from_bboxes.py#L171). | `masks.reshape(len(boxes), *masks.shape[-2:])` |
-| Every prompt gets a **full-frame** mask: `_predict` upsamples to `self._orig_hw`, then `predict()` does `.float().cpu().numpy()` | At 1920×1080 that is 8.29 MB per prompt in float32 — **829 MB on CPU for a 100-crab frame**, and 100 prompts is the exact worst case, since the detector caps detections per image at 100 (§2) | Chunk the prompts (`max_prompts_per_batch`, default 32 → 265 MB peak) and paint each chunk into the label image before releasing it |
+### 5. Configuration: a separate mask config
 
-Also: skip SAM2 entirely on frames with zero tracked boxes, rather than calling `set_image` and
-then `predict` with an empty array.
+The three knobs go in a **new config file of their own**, not into the tracking config:
 
-### 6. Configuration and the `sam2` dependency
+```yaml
+# crabs/tracker/config/mask_config.yaml   ✨ new file
+sam2_model_id: facebook/sam2.1-hiera-base-plus   # matches the existing script's default
+max_prompts_per_batch: 32                        # see §7
+shard_n_planes: 128                              # see §2; null disables sharding
+```
 
-The two knobs go in the **existing tracking config**, so the new entry point can reuse
-`tracking_parse_args` verbatim and adds **no CLI code**:
+**Why not a `masks:` block in `tracking_config.yaml`.** Nothing would break if it went there —
+config keys are read individually rather than splatted, so `Sort` is built from three named keys
+([track_video.py:155-159](../crabs/tracker/track_video.py#L155-L159)) and an unknown key is inert.
+The reasons are about shape, not safety:
+
+- [`tracking_config.yaml`](../crabs/tracker/config/tracking_config.yaml) is currently **four flat
+  SORT scalars**. A `masks:` block would be the first nested structure in it, and would put two
+  models' settings in one file for an entry point that only uses one of them.
+- The two have unrelated tuning lifecycles. *Points to discuss* anticipates comparing `-tiny` /
+  `-small` / `-base-plus`, which is mask-config churn that should never touch SORT parameters.
+- It removes a wart rather than documenting one: the integration test fetches its
+  `tracking_config.yaml` from the pooch/GIN registry
+  ([test_inference.py:45](../tests/test_integration/test_inference.py#L45)) and **that file has no
+  `masks:` key**, so a merged block would have to be defensively defaulted. A separate file with
+  its own default path never has the problem.
+- It is the shape issue [#249](https://github.com/SainsburyWellcomeCentre/crabs-exploration/issues/249)
+  ("Uncouple pipeline steps") will want: a later `--tracking_output_dir` mode would pass a mask
+  config and no tracking config at all.
+
+**The new flag is added by inheriting the tracking parser.** `tracking_parse_args` currently builds
+its parser and parses in one function ([track_video.py:350-451](../crabs/tracker/track_video.py#L350-L451)).
+Splitting the building out into `tracking_parser` lets the new entry point reuse every tracking
+argument through argparse's own `parents` mechanism, and adds `--mask_config_file` alongside them:
+
+```diff
+--- a/crabs/tracker/track_video.py
++++ b/crabs/tracker/track_video.py
++def tracking_parser() -> argparse.ArgumentParser:
++    """Build a parser with the arguments for tracking.
++
++    The parser is defined with ``add_help=False`` so that it can be used as a
++    parent parser (see ``parents`` in argparse).
++    """
++    parser = argparse.ArgumentParser(add_help=False)
++    ...   # every add_argument call, unchanged
++    return parser
++
++
+ def tracking_parse_args(args):
+     """Parse command-line arguments for tracking."""
+-    parser = argparse.ArgumentParser()
+-    ...
++    parser = argparse.ArgumentParser(parents=[tracking_parser()])
+     return parser.parse_args(args)
+```
 
-```yaml
-# crabs/tracker/config/tracking_config.yaml   ✨ new block
-masks:
-  sam2_model_id: facebook/sam2.1-hiera-base-plus   # matches the existing script's default
-  max_prompts_per_batch: 32                        # see §5
+```python
+# crabs/tracker/track_and_mask_video.py
+DEFAULT_MASK_CONFIG = str(Path(__file__).parent / "config" / "mask_config.yaml")
+
+
+def mask_parse_args(args):
+    """Parse command-line arguments for tracking and masking."""
+    parser = argparse.ArgumentParser(parents=[tracking_parser()])
+    parser.add_argument(
+        "--mask_config_file",
+        type=str,
+        default=DEFAULT_MASK_CONFIG,
+        help=(
+            "Location of YAML config to control masking. "
+            "Default: "
+            "crabs-exploration/crabs/tracker/config/mask_config.yaml. "
+        ),
+    )
+    return parser.parse_args(args)
 ```
 
-Read it as `{**MASK_DEFAULTS, **self.config.get("masks", {})}`. This matters: the integration test
-fetches its `tracking_config.yaml` from the pooch/GIN registry
-([test_inference.py:45](../tests/test_integration/test_inference.py#L45)), and **that file has no
-`masks:` key**.
+This follows the shape the other entry points already have — one `*_parse_args(args)` per entry
+point, returning one namespace — so `main` and `app_wrapper` take a single `args` as everywhere
+else in the package, and `--mask_config_file` shows up in `detect-and-track-mask --help` next to
+the tracking options.
+
+The `add_help=False` on `tracking_parser` is what makes it usable as a parent: two parsers each
+defining `-h` would raise `ArgumentError` on the child. Both callers therefore wrap it in a real
+parser that adds the help option itself.
+
+`tracking_parse_args` is only called from `app_wrapper` in the same module, so the split has no
+other callers to update.
+
+Read the file as `{**MASK_DEFAULTS, **yaml.safe_load(open(args.mask_config_file))}`. The
+defaults dict is still needed, so that a user's older config missing `shard_n_planes` does not
+fail.
 
+
+### 6. Dependencies
 On dependencies:
 
-- **`sam-2` is not on PyPI.** The only install route is the git URL used in
-  [generate_masks_from_bboxes.py:35](../scripts/generate_masks_from_bboxes.py#L35). A PEP 508
-  direct reference in `pyproject.toml` would make the sdist built by
-  [test_and_deploy.yml:59](../.github/workflows/test_and_deploy.yml#L59) unpublishable. It is
-  Apache-2.0, so there is no licence conflict — but it should be a **documented optional install
-  with a lazy import inside `generate_masks`**, raising an actionable error, not a declared
-  dependency. CI runs the unit suite on ubuntu and macOS without it.
+- **`sam-2` is not on PyPI, and a plain `pip install` from git pulls a second torch.** The only
+  source is the git URL used in
+  [generate_masks_from_bboxes.py:35](../scripts/generate_masks_from_bboxes.py#L35). SAM2's own
+  [`pyproject.toml`](https://github.com/facebookresearch/sam2/blob/main/pyproject.toml) declares
+  `torch>=2.5.1` as a **build** requirement (its `setup.py` imports `torch.utils.cpp_extension` to
+  build the CUDA extension), so `pip install "sam-2 @ git+..."` downloads an entire extra torch
+  into the isolated build environment — possibly a different variant from the one already
+  installed. That, not the clone, is the slow surprise on the cluster.
+
+  **So declare it as a PEP 735 dependency group, and disable build isolation for it:**
+
+  ```toml
+  # pyproject.toml
+  [dependency-groups]
+  masks = ["sam-2 @ git+https://github.com/facebookresearch/sam2.git"]
+
+  [tool.uv]
+  no-build-isolation-package = ["sam-2"]   # build sam-2 against the env's torch
+  ```
+
+  Installed with `uv sync --group masks` (or `pip install --group masks` on pip ≥ 25.1 — the
+  pips on this machine are older, so I have not run that form).
+
+  Dependency groups are **never written into distribution metadata**, so the sdist built by
+  [test_and_deploy.yml:59](../.github/workflows/test_and_deploy.yml#L59) is untouched — which is
+  what a PEP 508 direct reference in `[project.dependencies]` could not have promised. Everything
+  else about §6 stands: the group is opt-in, the lazy import inside `generate_masks` still raises
+  the actionable error, nothing in the default install pulls SAM2, and CI keeps running the unit
+  suite on ubuntu and macOS without it.
+
+  <details>
+  <summary>Two alternatives, and why not</summary>
+
+  - **An optional extra plus `[tool.uv.sources]`.** Checked rather than assumed: built with
+    uv 0.7.15, both the wheel and the sdist come out carrying `Requires-Dist: sam-2; extra ==
+    "masks"` — the URL *is* stripped, so the sdist would stay publishable. But the published
+    package would then advertise a `sam-2` that PyPI cannot resolve, so `pip install crabs[masks]`
+    fails for anyone not building from this repo. The group has neither problem.
+  - **A prebuilt wheel.** `sam_2-1.0.dist-info/direct_url.json` in the local `.venv` shows the
+    copy currently installed here came from
+    `https://github.com/horsto/sam2/releases/download/v0.0.2/sam_2-1.0-py3-none-any.whl` — pure
+    Python, no build step and no torch build dependency at all. The simplest of the three, at the
+    cost of trusting a third-party fork's release artefact rather than Meta's repo. Worth keeping
+    in mind if the source build proves painful on the cluster.
+  </details>
+
+- **Licence: Apache-2.0 for both the code and the weights.** The `sam-2` package is Apache-2.0
+  (read from the installed `dist-info`), and the checkpoints are too: the Hugging Face model cards
+  for `facebook/sam2.1-hiera-tiny`, `-small`, `-base-plus` and `-large` all declare
+  `license: apache-2.0` in their card metadata (all four checked on 2026-09-11). So there is no
+  licence conflict on either the code or the weights, and none of the `-tiny`/`-small`/`-base-plus`
+  swaps in *Points to discuss* #3 changes that.
+
+- **The conda/pip path still needs a plain command.** [README.md:33-42](../README.md#L33-L42) and
+  the HPC guides install with conda + `pip install -e .[dev]`, and `uv` appears in neither the
+  guides nor [test_and_deploy.yml](../.github/workflows/test_and_deploy.yml). The README section
+  therefore documents both routes: `uv sync --group masks` for a uv checkout, and, for a conda
+  environment with torch already installed,
+
+  ```bash
+  pip install --no-build-isolation "sam-2 @ git+https://github.com/facebookresearch/sam2.git"
+  ```
+
 - **`zarr` is imported directly but not declared** — [`crabs/zarr/create_dataset.py:21`](../crabs/zarr/create_dataset.py#L21)
   relies on it arriving transitively via `movement`. This proposal adds a second direct importer,
-  so it is worth declaring now (`zarr` 3.x is what `uv.lock` resolves).
+  so it is worth declaring now, as `zarr>=3` — §2's sharding needs 3.x. (A local `uv.lock` here
+  resolves 3.x, but that file is gitignored ([.gitignore:110](../.gitignore#L110)) and untracked,
+  so it is not evidence about anyone else's environment.)
 
+### 7. Three SAM2 details that will silently degrade or crash this
+
+| Detail | Consequence | Handling |
+|---|---|---|
+| `cv2.VideoCapture.read()` returns **BGR**; `set_image` documents **RGB** (`sam2_image_predictor.py:86-98`) | Silently worse masks — no error, no warning. The existing script never hit this because it reads RGB PNGs via PIL. | `cv2.cvtColor(frame, cv2.COLOR_BGR2RGB)` |
+| `predict()` ends with `masks.squeeze(0)`: returns `(N, 1, H, W)` for N>1 but `(1, H, W)` for N==1 | Crash or wrong axis on any frame with exactly one tracked crab. The same latent bug is in [generate_masks_from_bboxes.py:171](../scripts/generate_masks_from_bboxes.py#L171). | `masks.reshape(len(boxes), *masks.shape[-2:])` |
+| Every prompt gets a **full-frame** mask: `_predict` upsamples to `self._orig_hw`, then `predict()` does `.float().cpu().numpy()` | At 1920×1080 that is 8.29 MB per prompt in float32 — **829 MB on CPU for a 100-crab frame**, and 100 prompts is the exact worst case, since the detector caps detections per image at 100 (§2) | Chunk the prompts (`max_prompts_per_batch`, default 32 → 265 MB peak) and write each chunk's planes to zarr before releasing it |
+
+Also: skip SAM2 entirely on frames with zero tracked boxes, rather than calling `set_image` and
+then `predict` with an empty array.
 
 **Explicitly out of scope:** no ellipse fitting, no orientation, no angle in the CSV, no changes to
 the detector or to SORT. The masks are the deliverable; geometry comes later, offline, from the
@@ -258,40 +436,57 @@ mask store.
 
 ## Detailed implementation
 
-### The 6 changes
+### The 7 changes
 
 | # | Change | Signature / diff |
 |---|---|---|
 | 1 | **new** `crabs/tracker/track_and_mask_video.py` — the whole feature, ~150 lines | see below |
-| 2 | [`crabs/tracker/track_video.py`](../crabs/tracker/track_video.py) — expose the tracked boxes | `+ self.tracked_bboxes_dict = tracked_bboxes_dict` (one line, §1) |
-| 3 | [`crabs/tracker/config/tracking_config.yaml`](../crabs/tracker/config/tracking_config.yaml) | `masks:` block (§6) |
-| 4 | [`pyproject.toml`](../pyproject.toml) | `detect-and-track-mask = "crabs.tracker.track_and_mask_video:app_wrapper"`; declare `zarr` |
-| 5 | **new** `tests/test_unit/test_track_and_mask_video.py` | unit tests for the two pure helpers (no `sam2` needed) |
-| 6 | [`crabs/tracker/README.md`](../crabs/tracker/README.md) + [`guides/DetectAndTrackHPC.md`](../guides/DetectAndTrackHPC.md) | how to install `sam-2` and read the store back |
+| 2 | **new** `crabs/tracker/utils/masks.py` — the read-side helper, ~25 lines | `to_label_image` (§3) |
+| 3 | [`crabs/tracker/track_video.py`](../crabs/tracker/track_video.py) — expose the tracked boxes, and the parser | `+ self.tracked_bboxes_dict = tracked_bboxes_dict` (one line, §1); split `tracking_parse_args` into `tracking_parser()` + `parse_args` (§5) |
+| 4 | **new** `crabs/tracker/config/mask_config.yaml` | the three SAM2/store knobs (§5) |
+| 5 | [`pyproject.toml`](../pyproject.toml) | `detect-and-track-mask = "crabs.tracker.track_and_mask_video:app_wrapper"`; declare `zarr>=3`; add the `masks` dependency group and `[tool.uv] no-build-isolation-package` (§6) |
+| 6 | **new** `tests/test_unit/test_track_and_mask_video.py` | unit tests for the pure helpers (no `sam2` needed) |
+| 7 | [`crabs/tracker/README.md`](../crabs/tracker/README.md) + [`guides/DetectAndTrackHPC.md`](../guides/DetectAndTrackHPC.md) | how to install the `masks` group (§6), pass `--mask_config_file`, and read the store back |
+
 
 <details>
 <summary><b>1. The new module in full outline</b></summary>
 
-Four module-level functions plus a four-method subclass. The first two functions are pure — no
-`sam2`, no torch, no I/O beyond zarr — which is what makes change #5 possible on CI.
+Five module-level functions plus a subclass, with the read-side helper in `utils/masks.py`.
+Everything except `load_sam2_predictor` and `predict_masks_for_frame` is pure — no `sam2`, no
+torch, no I/O beyond zarr and the config file — which is what makes the unit tests possible on CI.
 
 ```python
 """Detect, track and mask crabs in a video."""
 
+DEFAULT_MASK_CONFIG = str(Path(__file__).parent / "config" / "mask_config.yaml")
+
 MASK_DEFAULTS = {
     "sam2_model_id": "facebook/sam2.1-hiera-base-plus",
     "max_prompts_per_batch": 32,
+    "shard_n_planes": 128,          # §2; None disables sharding
 }
 
 
-def paint_masks_as_label_image(masks, track_ids, image_shape) -> np.ndarray:
-    """(N, H, W) bool + (N,) ids -> (H, W) int32 label image.  [§3]"""
+def mask_parse_args(args):
+    """Parse command-line arguments for tracking and masking.  [§5]"""
+    parser = argparse.ArgumentParser(parents=[tracking_parser()])   # inherits every tracking arg
+    parser.add_argument("--mask_config_file", type=str, default=DEFAULT_MASK_CONFIG, help=...)
+    return parser.parse_args(args)
 
 
-def create_mask_zarr(path, n_frames, image_shape, metadata_dict) -> zarr.Array:
-    """Open an (T, H, W) int32 store, chunks=(1, H, W), and write .attrs.
+def load_mask_config(path) -> dict:
+    """Read the mask config, backfilled with MASK_DEFAULTS.  [§5]"""
+    with open(path) as f:
+        return {**MASK_DEFAULTS, **(yaml.safe_load(f) or {})}
 
-    Asserts the array dtype is integer, matching metadata["mask_encoding"].
+
+def create_mask_zarr(path, n_frames, n_track_ids, image_shape, metadata_dict) -> zarr.Array:
+    """Open a (T, M, H, W) bool array named "masks" in a group at `path`.
+
+    chunks=(1, 1, H, W), shards=(1, shard_n_planes, H, W), fill_value=False.
+    Asserts the array dtype is bool, matching metadata["mask_encoding"]
+    == "instance_planes" — the guard that keeps §2's warning from recurring.
     """
 
 
@@ -302,7 +497,9 @@ def load_sam2_predictor(model_id: str, device: str):
     except ImportError as e:
         raise ImportError(
             "detect-and-track-mask needs SAM2. Install it with:\n"
-            '  pip install "sam-2 @ git+https://github.com/facebookresearch/sam2.git"'
+            "  uv sync --group masks\n"
+            "or, in a conda env with torch already installed:\n"
+            '  pip install --no-build-isolation "sam-2 @ git+https://github.com/facebookresearch/sam2.git"'
         ) from e
     return SAM2ImagePredictor.from_pretrained(model_id, device=device)
 
@@ -320,12 +517,16 @@ def predict_masks_for_frame(predictor, frame_bgr, boxes, max_prompts_per_batch):
 class TrackingAndMasking(Tracking):
     """Extend Tracking with a SAM2 masking pass over the tracked boxes."""
 
-    def prep_mask_outputs(self): ...     # path + config defaults
+    def __init__(self, args, mask_config):
+        super().__init__(args)           # unchanged; reads the tracking config
+        self.mask_config = mask_config   # separate file, §5
+
+    def prep_mask_outputs(self): ...     # output path + M (below)
     def generate_masks(self): ...        # the loop, below
 
 
 def main(args):
-    inference = TrackingAndMasking(args)
+    inference = TrackingAndMasking(args, load_mask_config(args.mask_config_file))
     inference.detect_and_track_video()   # inherited, unchanged
     inference.generate_masks()
 
@@ -333,7 +534,7 @@ def main(args):
 def app_wrapper():
     logging.getLogger().setLevel(logging.INFO)
     torch.set_float32_matmul_precision("medium")
-    main(tracking_parse_args(sys.argv[1:]))   # reuses the existing parser verbatim
+    main(mask_parse_args(sys.argv[1:]))   # same shape as the other entry points, §5
 ```
 
 `generate_masks`, mirroring the structure of `write_all_video_frames_as_images`
@@ -342,7 +543,12 @@ def app_wrapper():
 ```python
 def generate_masks(self):
     predictor = load_sam2_predictor(cfg["sam2_model_id"], self.accelerator)
-    mask_zarr = create_mask_zarr(...)
+
+    # M is knowable here: tracking has already finished  [§2]
+    n_track_ids = max(
+        frame["ids"].max() for frame in self.tracked_bboxes_dict.values() if len(frame["ids"])
+    )
+    mask_zarr = create_mask_zarr(path, total_n_frames, n_track_ids, (H, W), metadata)
 
     input_video_object = open_video(self.input_video_path)
     frame_idx = 0
@@ -356,21 +562,24 @@ def generate_masks(self):
         boxes, ids = frame_data["tracked_boxes"], frame_data["ids"]
         IF len(boxes) > 0:                                    # [§5]
             masks = predict_masks_for_frame(predictor, frame, boxes, chunk_size)
-            mask_zarr[frame_idx] = paint_masks_as_label_image(
-                masks, ids.astype(int), mask_zarr.shape[1:]
-            )
+            mask_zarr.oindex[frame_idx, ids.astype(int) - 1] = masks   # [§3]
         frame_idx += 1
 
     input_video_object.release()
 ```
 
-Frames with no tracked boxes are left at the store's `fill_value=0` — correct by construction, and
-one fewer branch.
+Frames with no tracked boxes are left at the store's `fill_value=False`, and those chunks are never
+written — correct by construction, and one fewer branch.
+
+Two invariants worth asserting at the end of the run, both cheap and both about the ID↔axis
+mapping the format now depends on: every emitted track ID is in `1..M`, and
+`KalmanBoxTracker.count >= M` (it can exceed `M` when a tracker was suppressed below `min_hits`,
+never fall below it).
 
 **Rejected alternative — masking inside `core_detection_and_tracking`.** It saves one video pass,
 but it puts SAM2 in the middle of the detection loop, changes an existing method, and makes the
 feature impossible to skip. A fourth pass matches the pattern the file already uses twice and
-keeps the diff to one added line in existing code.
+keeps the diff to `track_video.py` down to the one added line.
 
 **Rejected alternative — a post-pass over an existing `_tracks.csv`.** More decoupled, and
 attractive later, but it means re-parsing VIA JSON and re-deriving the frame index from filenames
@@ -378,22 +587,52 @@ when the same data is already in memory. It also could not reuse `Tracking` at a
 revisiting once the format is settled.
 </details>
 
+<details>
+<summary><b>2. Formats considered and rejected</b></summary>
+
+All four are lossless-or-not on overlap, and all four are `regionprops`-shaped at read time. The
+choice came down to what the downstream access pattern is.
+
+| Format | Lossless on overlap | `masks[:, tid]` per-crab slice | Opens in napari as-is | Note |
+|---|---|---|---|---|
+| **`(T, M, H, W)` bool, plane per track ID** ✅ chosen | yes | **yes, one slice** | no | chunk count is the cost (§2) |
+| `(T, H, W)` int32 label image | **no** | no | yes | simplest; discards the lower-ID crab's overlapping pixels |
+| `(T, K, H, W)` int32 overlap layers | yes | no | plane 0 ≈ whole frame | `K`≈2–4; needs a greedy packing pass at write time |
+| bbox-local crops + index table | yes | no (needs a scan) | no | ~500× smaller raw, but ragged and no time-slicing |
+
+**Why not the label image.** It was the original proposal. It cannot represent two crabs on one
+pixel, and the ellipse fits Phase 3 wants would be biased by exactly the neighbours that touch
+most. Rejected on losslessness, not on size — zarr compresses a label image well enough that size
+was never the deciding argument.
+
+**Why not overlap layers.** Genuinely close, and better on chunk count and napari. It loses because
+the axis carries no identity, so `masks[:, tid]` — one crab's whole trajectory in a single read —
+is not expressible. That slice is what the orientation work will be built on.
+
+**Why not crops.** Much the smallest, and the only one that scales to long clips without thought.
+Rejected for now because it is ragged (two arrays plus offset bookkeeping), needs a scan to
+assemble one crab's time series, and cannot be read without the repo's helpers. Worth revisiting
+if *Points to discuss* #1 or #2 comes back badly.
+</details>
+
 ---
 
 ## Files changed
 
 | File | Change |
 |---|---|
-| `crabs/tracker/track_and_mask_video.py` | **new** — the entire feature |
-| [`crabs/tracker/track_video.py`](../crabs/tracker/track_video.py) | one added line (store the tracked boxes on `self`) |
-| [`crabs/tracker/config/tracking_config.yaml`](../crabs/tracker/config/tracking_config.yaml) | `masks:` block |
-| [`pyproject.toml`](../pyproject.toml) | new `detect-and-track-mask` script; declare `zarr` |
+| `crabs/tracker/track_and_mask_video.py` | **new** — the entire write path |
+| `crabs/tracker/utils/masks.py` | **new** — `to_label_image` (§3) |
+| [`crabs/tracker/track_video.py`](../crabs/tracker/track_video.py) | one added line (store the tracked boxes on `self`); split `tracking_parse_args` into `tracking_parser()` + `parse_args`, so the new entry point can inherit it with `parents=[...]` (§5) |
+| `crabs/tracker/config/mask_config.yaml` | **new** — SAM2 and store knobs, separate from the tracking config (§5) |
+| [`pyproject.toml`](../pyproject.toml) | new `detect-and-track-mask` script; declare `zarr>=3`; `[dependency-groups] masks` + `[tool.uv] no-build-isolation-package` (§6) |
 | `tests/test_unit/test_track_and_mask_video.py` | **new** |
-| [`crabs/tracker/README.md`](../crabs/tracker/README.md) | document the entry point, the store layout and how to read it back |
-| [`guides/DetectAndTrackHPC.md`](../guides/DetectAndTrackHPC.md) | installing `sam-2` on the cluster |
+| [`notebooks/notebook_visualise_masks_from_zarr.py`](../notebooks/notebook_visualise_masks_from_zarr.py) | drop the one-hot `map_blocks` expansion ([:142-174](../notebooks/notebook_visualise_masks_from_zarr.py#L142-L174)) — the store is already 4-D; route the two napari calls through `to_label_image` (§3) |
+| [`crabs/tracker/README.md`](../crabs/tracker/README.md) | document the entry point, `--mask_config_file`, the store layout and how to read it back |
+| [`guides/DetectAndTrackHPC.md`](../guides/DetectAndTrackHPC.md) | installing the `masks` group on the cluster (and the `--no-build-isolation` pip line for the conda envs, §6); staging the second config file |
 
-Nothing else is touched. In particular: no change to `sort.py`, to `utils/io.py`, to the CSV
-format, to `evaluate_tracker.py`, or to the detector.
+Nothing else is touched. In particular: no change to `sort.py`, to `utils/io.py`, to
+`tracking_config.yaml`, to the CSV format, to `evaluate_tracker.py`, or to the detector.
 
 ---
 
@@ -401,41 +640,60 @@ format, to `evaluate_tracker.py`, or to the detector.
 
 **Pure unit — must pass with no `sam2` installed (this is the CI shape)**
 
-1. `paint_masks_as_label_image`: three non-overlapping masks with IDs `[7, 3, 12]` produce a label
-   image whose non-zero values are exactly `{3, 7, 12}` and whose regions are in the right places.
-2. Overlap policy: two overlapping masks with IDs `3` and `12` — the shared pixels are `12`, and
-   the result is identical when the inputs are passed in the opposite order (the `argsort` is doing
-   its job).
-3. Empty input `(0, H, W)` returns an all-zero image and does not raise.
-4. Round-trip through the consumer: feed the output of #1 to `skimage.measure.regionprops` and
-   assert `{p.label for p in props} == {3, 7, 12}`. This is the property the whole format exists
-   for, so it should be asserted rather than assumed.
-5. `create_mask_zarr`: the array dtype is integer, `fill_value` is 0, chunks are `(1, H, W)`, and
-   `.attrs["mask_encoding"] == "track_id"` — plus a test that the internal assertion **fires** if
-   the dtype and the declared encoding disagree (the guard against the `dtype="bool"` bug).
-6. `load_sam2_predictor` raises `ImportError` with the install command in the message when `sam2`
-   is absent (`monkeypatch` the import).
-7. The track-ID/dtype guard (§2): a track ID exceeding `np.iinfo(dtype).max` raises rather than
-   wrapping around silently. Cheap to test directly on the helper, and it is the assertion that
-   makes a later switch to `uint16` safe.
+1. **The ID↔axis mapping, which is now the core contract.** Write three masks with IDs `[7, 3, 12]`
+   via `oindex[t, ids - 1]`, then assert `masks[t, 6]`, `masks[t, 2]` and `masks[t, 11]` are the
+   masks that went in, and that every other plane in frame `t` is all-`False`.
+2. **Overlap is preserved.** Two masks with IDs `3` and `12` sharing a block of pixels: both planes
+   still contain the shared pixels in full, and each plane's `sum()` equals its input's. This is
+   the test that would fail under the label-image format, and it is the reason for the change.
+3. `to_label_image`: non-overlapping planes give a label image whose non-zero values are exactly
+   `{3, 7, 12}`, regions in the right places; overlapping planes resolve by `policy`, and the
+   result does not depend on plane order.
+4. Round-trip through the consumer: feed `to_label_image` output to `skimage.measure.regionprops`
+   and assert `{p.label for p in props} == {3, 7, 12}`. The property the read path exists for.
+5. A frame with no tracked boxes leaves every plane all-`False` and does not raise.
+6. `create_mask_zarr`: dtype is `bool`, `fill_value` is `False`, chunks are `(1, 1, H, W)`, shards
+   are `(1, 128, H, W)`, and `.attrs["mask_encoding"] == "instance_planes"` — plus a test that the
+   internal assertion **fires** if the dtype and the declared encoding disagree (the guard that
+   keeps §2's `dtype="bool"` bug from being reintroduced from the other direction).
+7. `load_sam2_predictor` raises `ImportError` with the install command in the message when `sam2`
+   is absent (`monkeypatch` the import) — assert the message names `uv sync --group masks`, so it
+   cannot drift from the group actually declared in `pyproject.toml`.
+8. **`mask_parse_args` (§5).** Three cases, since this is the only new CLI surface: the namespace
+   carries `mask_config_file` when the flag is passed, and `DEFAULT_MASK_CONFIG` when it is not;
+   every tracking argument is inherited (assert that `vars(mask_parse_args(argv + mask_flag))`
+   minus `mask_config_file` equals `vars(tracking_parse_args(argv))` for the same `argv`); and
+   `--help` exits `0` with both `--mask_config_file` and `--config_file` in the usage text — the
+   property the `parents=[...]` refactor buys, and the one that would regress if
+   `tracking_parser` were ever given `add_help=True`.
+9. `load_mask_config`: a config missing `shard_n_planes` is backfilled from `MASK_DEFAULTS`, and
+   an empty file yields the defaults rather than raising — the guard against
+   `yaml.safe_load` returning `None`.
 
 **Existing tests that must stay green, unmodified**
 
-8. `pytest tests/test_unit` — in particular
+10. `pytest tests/test_unit` — in particular
    [`test_tracking_io.py`](../tests/test_unit/test_tracking_io.py) (the CSV is untouched) and
-   [`test_track_video.py`](../tests/test_unit/test_track_video.py) (the one-line change to
-   `detect_and_track_video` must not alter behaviour).
-9. [`test_entry_points.py`](../tests/test_unit/test_entry_points.py) — extend with
+   [`test_track_video.py`](../tests/test_unit/test_track_video.py) (neither the one-line change to
+   `detect_and_track_video` nor the parser split must alter behaviour). The parser split has no
+   existing test covering it — `tracking_parse_args` is called only from `app_wrapper` — so add
+   one: `tracking_parse_args` on a minimal valid argv still returns the same defaults, and
+   `detect-and-track-video --help` still lists every tracking option.
+11. [`test_entry_points.py`](../tests/test_unit/test_entry_points.py) — extend with
    `detect-and-track-mask` following the existing pattern.
 
 **Integration (slow, opt-in)**
 
-10. A `test_detect_and_track_mask` alongside
+12. A `test_detect_and_track_mask` alongside
    [`test_detect_and_track_video`](../tests/test_integration/test_inference.py), reusing the
    `pooch_registry` fixture and the 3-frame clip, `@pytest.mark.skipif` on `sam2` being importable.
-   Assert the store exists, has shape `(3, H, W)`, and that the set of non-zero labels in each
-   frame equals the set of track IDs in that frame's rows of the CSV. That last assertion is the
-   real contract of this feature.
+   Assert the store exists, has shape `(3, M, H, W)` and dtype `bool`, and that for each frame the
+   set of planes with any `True` pixel is exactly `{tid - 1 for tid in that frame's CSV rows}`.
+   That last assertion is the real contract of this feature.
+
+   Note this test now passes the registry's `tracking_config.yaml` **unchanged** and lets
+   `--mask_config_file` fall back to its packaged default — with a separate config there is nothing
+   to add to the GIN registry file (§5).
 
 ---
 
@@ -444,7 +702,7 @@ format, to `evaluate_tracker.py`, or to the detector.
 ```bash
 # lint + full unit suite (no sam2 needed)
 pre-commit run --all-files
-pytest tests/test_unit
+pytest tests
 
 # slow end-to-end CLI tests (pooch downloads test data on first run)
 pytest -m slow tests/test_integration/test_inference.py
@@ -453,11 +711,13 @@ pytest -m slow tests/test_integration/test_inference.py
 Manual check on a real clip:
 
 ```bash
-pip install "sam-2 @ git+https://github.com/facebookresearch/sam2.git"
+uv sync --group masks   # §6; in a conda env instead:
+# pip install --no-build-isolation "sam-2 @ git+https://github.com/facebookresearch/sam2.git"
 
 detect-and-track-mask \
     --trained_model_path <ckpt> \
     --video_path <clip.mp4> \
+    --mask_config_file <mask_config.yaml> \   # optional; defaults to the packaged one
     --config_file <tracking_config.yaml> \
     --accelerator=gpu \
     --output_dir_no_timestamp
@@ -468,77 +728,99 @@ then confirm the contract and eyeball the masks:
 ```python
 import zarr, numpy as np
 from skimage.measure import regionprops
+from crabs.tracker.utils.masks import to_label_image
+
+root = zarr.open("tracking_output/<clip>_masks.zarr", mode="r")
+masks = root["masks"]
+print(masks.shape, masks.dtype, dict(root.attrs))   # (T, M, H, W) bool
 
-masks = zarr.open("tracking_output/<clip>_masks.zarr", mode="r")
-print(masks.shape, masks.dtype, dict(masks.attrs))
+# the contract: which planes are populated in frame 0
+print(sorted(np.flatnonzero(masks[0].any(axis=(1, 2))) + 1))   # == track IDs in frame 0 of the CSV
 
-props = regionprops(masks[0])
-print(sorted(p.label for p in props))     # must match the track IDs in <clip>_tracks.csv frame 0
+# the read path
+props = regionprops(to_label_image(masks[0]))
 print([p.area for p in props][:10])       # sanity: no zero-area or whole-frame regions
 ```
 
 [`notebooks/notebook_visualise_masks_from_zarr.py`](../notebooks/notebook_visualise_masks_from_zarr.py)
-already loads this shape into napari; overlaying it on the frames written by `--save_frames` is the
-fastest qualitative check that the masks are on the crabs and not on the background.
+loads this shape into napari once its one-hot expansion is dropped (*Files changed*); overlaying it
+on the frames written by `--save_frames` is the fastest qualitative check that the masks are on the
+crabs and not on the background.
 
-Also worth recording once, since it decides the format's viability: `du -sh` of the store for one
-real clip, and the wall-clock time of the masking pass versus the detection pass.
+Also worth recording once, since both decide the format's viability: `du -sh` **and the file count**
+(`find ... | wc -l`) of the store for one real clip, and the wall-clock time of the masking pass
+versus the detection pass.
 
 ---
 
 ## Points to discuss
 
-1. **Is the overlap loss acceptable?** A single label image cannot represent two crabs occupying
-   the same pixel, and this scene has ~100 frequently touching individuals — so the lower-ID crab's
-   overlapping pixels are discarded (§3). Ellipse fits on a partly-eaten mask will be biased. The
-   alternative is storing **per-instance** masks (COCO RLE via `pycocotools`, already a dependency,
-   keyed by frame and track ID), which is lossless but is no longer directly `regionprops`-shaped —
-   it needs a `mask.decode()` first. My recommendation is to start with the label image, since it
-   is simpler and matches the existing repo convention, and measure how many pixels are actually
-   lost before deciding — that measurement is one extra line in `paint_masks_as_label_image` and
-   could be stored as a per-frame attr. Worth adding now?
-2. **Store size.** `(T, H, W)` int32 at 1920×1080 is 8.29 MB/frame uncompressed, so ~25 GB raw for
-   a 3000-frame clip. Label images are sparse and blocky so zarr's default compressor should reduce
-   that by a large factor, but I have not measured it and will not guess. If it turns out too big,
-   the levers are, in order: `uint16` (2×, at the cost of a 65535 track-ID ceiling — reachable in
-   principle, since the 100-detections-per-image cap only bounds IDs at `100 × n_frames`; see §2),
-   a stronger compressor, or the per-instance-RLE format from point 1 (which is far smaller because
-   it never stores background).
+1. **Chunk count is this format's one weak spot, and it is unmeasured.** Per-instance planes mean
+   ~`n_frames × n_crabs` ≈ 300,000 chunks for a 3000-frame clip (§2). Sharding at
+   `(1, 128, H, W)` should cut that to `n_frames × ceil(M/128)`, but I have not measured either the
+   file count or whether zarr 3's sharding writer behaves well with `oindex` writes of a frame's
+   scattered IDs — it may do a read-modify-write per shard. This is the number I would check first
+   on the cluster, and `shard_n_planes` is a config value precisely so it can be tuned without a
+   format change. If sharding disappoints, the fallback is a zip store.
+2. **Store size.** Raw `(T, M, H, W)` bool is enormous — 622 GB for `T=3000, M=100` — but it is
+   never stored: each plane is ~0.17% non-zero, all-`False` chunks are not written at all, and only
+   ~`n_frames × n_crabs` chunks ever exist. My estimate is the same order as the label image would
+   have compressed to (a few hundred MB to ~1 GB), but I have not measured it and will not guess.
+   Note this format has no `uint16`/`int32` lever to pull because there is no ID in the data — if
+   it comes out too big the levers are a stronger compressor, or storing bbox-local crops rather
+   than full-frame planes (which would drop the raw volume by ~500× but costs the direct
+   `masks[:, tid-1]` slicing that motivated this shape).
 3. **`sam2_model_id` default.** I have matched the existing script's `-base-plus`
    ([generate_masks_from_bboxes.py:307](../scripts/generate_masks_from_bboxes.py#L307)) so the two
    agree. `-tiny` / `-small` are considerably faster and may well be enough at this object size —
-   easy to compare once the entry point exists, since it is a config value.
-4. **Should the entry point re-run detection, or accept an existing tracking output?** As proposed
-   it re-runs everything, which is the simplest thing and matches the name. The cost is that
-   iterating on SAM2 settings means re-running the detector each time. A `--tracking_output_dir`
+   easy to compare once the entry point exists, since it is a config value, and with the separate
+   mask config (§5) swapping it never touches the tracking parameters.
+
+5. **Should the entry point re-run detection, or accept an existing tracking output?**
+    * As proposed
+   it re-runs everything, which is the simplest thing and matches the name.
+   * The cost is that
+   iterating on SAM2 settings means re-running the detector each time.
+   * A `--tracking_output_dir`
    flag that skips straight to `generate_masks` would fix that and fits issue
    [#249](https://github.com/SainsburyWellcomeCentre/crabs-exploration/issues/249)
    ("Uncouple pipeline steps"), but it needs the tracks CSV parsed back in. Deliberately left out;
-   easy to add later without changing the format.
-5. **Not verified: the SAM2 *checkpoint* licence.** The `sam-2` package is Apache-2.0 (read from
-   the installed `dist-info`), and I assumed the `facebook/sam2.1-hiera-*` weights on Hugging Face
-   are too — but I did not open the model card.
-6. **⚠️ The detector is running at its detection cap.** `fasterrcnn_resnet50_fpn_v2` is constructed
+   easy to add later without changing the format — and the separate mask config (§5) is already the
+   right shape for it, since that mode would pass a mask config and no tracking config at all.
+
+6. **⚠️ The detector is running at its detection cap.**
+    * `fasterrcnn_resnet50_fpn_v2` is constructed
    with no kwargs ([models.py:82](../crabs/detector/models.py#L82)), so torchvision's default
-   `box_detections_per_img=100` applies — and this scene has **~100 crabs per frame**. Dense frames
+   `box_detections_per_img=100` applies — and this scene has **~100 crabs per frame**.
+   * Dense frames
    are therefore plausibly being truncated to the top 100 by score, silently, before tracking and
-   before any masking. This does not change anything in this proposal — the masks follow whatever
+   before any masking.
+   * This does not change anything in this proposal — the masks follow whatever
    the tracker emits — but it caps what the masks can ever cover, and it is invisible in the
-   outputs. Worth its own issue and a quick check: log
-   `max(len(boxes) for boxes in detections)` over a real clip and see how often it sits at exactly
-   100.
-7. **Two more things found while reading, both out of scope.** (a) `--max_frames_to_read` is parsed at
+   outputs.
+   * Worth its own issue and a quick check: log
+   `max(len(boxes) for boxes in detections)` over a real clip and see how often it sits at exactly 100.
+
+7. **The dependency group only helps a uv checkout, and this repo is not one yet.**
+    * `uv.lock` is
+   gitignored and untracked, and the install docs are conda + pip throughout, so `uv sync --group
+   masks` documents a workflow the repo does not otherwise have — which is why §6 keeps the
+   `pip install --no-build-isolation` line alongside it.
+   * If you would rather commit `uv.lock` and
+   make uv the documented path (which would also pin the SAM2 commit, rather than tracking
+   whatever `main` is on the day someone installs), that is a bigger, separate decision and I have
+   not assumed it here.
+8. **Two more things found while reading, both out of scope.**
+    (a) `--max_frames_to_read` is parsed at
    [track_video.py:440](../crabs/tracker/track_video.py#L440) and never used by anything — see PR
    [#245](https://github.com/SainsburyWellcomeCentre/crabs-exploration/pull/245). It would make
-   iterating on this entry point much cheaper. (b) `write_tracked_detections_to_csv` zips the
+   iterating on this entry point much cheaper (because we could run things for a limited amount of frames).
+
+   (b) `write_tracked_detections_to_csv` zips the
    tracked boxes against `detections_dict["scores"]`, which holds *all raw, unthresholded*
    detections in detector order ([track_video.py:267](../crabs/tracker/track_video.py#L267),
    [io.py:77-82](../crabs/tracker/utils/io.py#L77-L82)) — different length and different order, so
-   the CSV `confidence` column is effectively arbitrary. Neither affects the masks; both are worth
-   separate PRs.
+   the CSV `confidence` column is effectively arbitrary.
 
----
-
-## Feedback on the format of this proposal
-
-If any section is too long, too short, or in the wrong order for how you actually review, say so.
+   Neither affects the masks; both are worth
+   separate PRs.

From f746245d7e93edf0b2b423e438c1c6c5216135e9 Mon Sep 17 00:00:00 2001
From: sfmig <33267254+sfmig@users.noreply.github.com>
Date: Wed, 16 Sep 2026 12:57:29 +0100
Subject: [PATCH 03/19] Fix output video script

---
 scripts/visualization/output_video.py | 58 ++++++++++++++++++++++++++-
 1 file changed, 56 insertions(+), 2 deletions(-)

diff --git a/scripts/visualization/output_video.py b/scripts/visualization/output_video.py
index 7199d908..4f9b7351 100644
--- a/scripts/visualization/output_video.py
+++ b/scripts/visualization/output_video.py
@@ -5,6 +5,7 @@
 
 import cv2
 import matplotlib.pyplot as plt
+import numpy as np
 from movement.io import load_bboxes
 from tqdm import tqdm
 
@@ -34,6 +35,50 @@ def get_distinct_colors():
     return colors
 
 
+def reindex_to_video_frames(ds, video_path):
+    """Pad the dataset's time coordinate to span every frame of the video.
+
+    `movement` pads with NaNs the individuals that are missing from a frame,
+    but a frame that no individual appears in is absent from the time
+    coordinate altogether. We reindex to add those frames back, so that row
+    `i` of the dataset is frame `i` of the video and the arrays can be
+    indexed positionally by frame number.
+
+    The dataset is expected to hold the frame numbers as written in the VIA
+    tracks file (i.e. loaded with `use_frame_numbers_from_file=True` and
+    `fps=None`), which the tracker numbers from 0 over the video it is given.
+    """
+    # Get n frames in video
+    cap = cv2.VideoCapture(str(video_path))
+    n_frames = int(cap.get(cv2.CAP_PROP_FRAME_COUNT))
+    cap.release()
+
+    # Check the frame numbers belong to this video. `reindex` discards
+    # out-of-range labels silently, so without this an unrelated tracks file
+    # would give an all-NaN dataset rather than an error.
+    frames_0idx = ds.time.values
+    if frames_0idx.min() < 0 or frames_0idx.max() >= n_frames:
+        raise ValueError(
+            f"{Path(video_path).name}: the frame numbers in the tracks file "
+            f"({frames_0idx.min()}-{frames_0idx.max()}) fall outside the "
+            f"video's frame range 0-{n_frames - 1}. Are the tracks from this "
+            "video?"
+        )
+
+    # Print number of frames with no detections
+    n_frames_wo_boxes = n_frames - ds.sizes["time"]
+    if n_frames_wo_boxes:
+        print(
+            f"{Path(video_path).name}: {n_frames_wo_boxes} of {n_frames} "
+            "frames have no tracked boxes; padding them with NaNs."
+        )
+
+    # Reindex dataset to number of frames in video
+    # (reindexing fills in any gaps with nans, but silently drops
+    # frames out of the specificied range; hence the range check earlier)
+    return ds.reindex(time=np.arange(n_frames))
+
+
 def create_opencv_video(
     ds,
     input_video,
@@ -172,10 +217,19 @@ def create_opencv_video(
         input_video = Path(input_data_dir) / f"{escape_clip_name}.mp4"
         pred_csv = Path(input_data_dir) / f"{escape_clip_name}_tracks.csv"
 
-        # Read predictions as a movement dataset
+        # Read predictions as a movement dataset.
+        # We request the frame numbers as defined in the file
+        # (only frames with detections are included in the file)
         ds_pred = load_bboxes.from_via_tracks_file(
-            pred_csv, fps=None, use_frame_numbers_from_file=False
+            pred_csv,
+            fps=None,
+            use_frame_numbers_from_file=True,
         )
+
+        # Pad the time coordinate to span the full video, so that row i of
+        # the dataset is frame i of the video
+        ds_pred = reindex_to_video_frames(ds_pred, input_video)
+
         list_individuals_idcs = list(range(len(ds_pred.individuals)))
 
         # Create prediction video

From 78968aef4e911a54e5b858189cd37038d45dd411 Mon Sep 17 00:00:00 2001
From: sfmig <33267254+sfmig@users.noreply.github.com>
Date: Wed, 16 Sep 2026 13:14:47 +0100
Subject: [PATCH 04/19] First reviewed proposal draft

---
 claude-plans/proposal-zarr-time-coordinate.md | 482 ++++++++++++++++++
 1 file changed, 482 insertions(+)
 create mode 100644 claude-plans/proposal-zarr-time-coordinate.md

diff --git a/claude-plans/proposal-zarr-time-coordinate.md b/claude-plans/proposal-zarr-time-coordinate.md
new file mode 100644
index 00000000..cd78985c
--- /dev/null
+++ b/claude-plans/proposal-zarr-time-coordinate.md
@@ -0,0 +1,482 @@
+# Proposal for anchoring the `time` coordinate of the tracks zarr dataset to the clip
+
+## Description
+
+The `time` coordinate of the tracks zarr dataset is not the clip's frame index, so `escape_state`
+marks the escape on the wrong frame. This proposal anchors `time` to the clip, in
+[`load_extended_ds`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L49).
+
+```mermaid
+flowchart TD
+    A["&lt;clip&gt;_tracks.csv<br/><i>rows only for frames with tracked crabs</i>"] --> B
+    B["load_bboxes.from_via_tracks_file<br/>✨ use_frame_numbers_from_file=True"] --> C
+    C["ds.time = real frame numbers<br/><i>frames with no crabs are still missing</i>"] --> D
+    D["✨ ds.reindex(time=arange(n_clip_frames))<br/><i>n_clip_frames = loop_END_frame_ffmpeg<br/>- loop_START_frame_ffmpeg + 1</i>"] --> E
+    E["ds.time = 0 … n_clip_frames-1<br/><b>row i is clip frame i</b>"] --> F
+    F["escape_state, over the full clip"] --> G
+    G["temp store: one group per clip"] --> H
+    H["xr.concat(dim='clip_id', join='outer')"] --> I
+    I["final store: one group per video"]
+    style B fill:#e0e7ff,stroke:#4338ca,color:#1e1b4b
+    style D fill:#e0e7ff,stroke:#4338ca,color:#1e1b4b
+    style E fill:#e0e7ff,stroke:#4338ca,color:#1e1b4b
+```
+
+Arrows are the data path of one clip, from its VIA tracks file to the final zarr store. ✨ marks
+what this pull request adds.
+
+Node → code:
+
+- `B`, `C`, `D`, `E`, `F` — [`load_extended_ds`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L49)
+- `G` — [`create_temp_zarr_store`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L203)
+- `H`, `I` — [`create_final_zarr_store`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L277)
+
+## References
+
+
+- [`guides/CreateZarrDatasetForTracks.md`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/guides/CreateZarrDatasetForTracks.md)
+  — how the dataset is built on the cluster.
+- [`movement.io.load_bboxes.from_via_tracks_file`](https://github.com/neuroinformatics-unit/movement/blob/main/movement/io/load_bboxes.py)
+  — the loader whose default behaviour partially causes the issue (to be changed in [PR1103](https://github.com/neuroinformatics-unit/movement/pull/1103)). Behaviour checked against movement 0.16.0, the 0.17.0
+  source, and `main`; identical in all three.
+- No open pull request touches `crabs/zarr/`.
+
+## Overview of steps
+
+1. Load each VIA tracks file with `use_frame_numbers_from_file=True`.
+2. Derive the clip's frame count from the metadata csv.
+3. Raise if the frame numbers in the file fall outside the clip's frame range.
+4. Reindex the dataset onto the clip's full frame range.
+5. Build `escape_state` over that frame range, and raise if the escape frame falls outside it.
+6. Strengthen `test_load_extended_ds` to assert the values of `escape_state`, not just its presence.
+7. Audit the VIA tracks files on the cluster to decide whether existing stores must be rebuilt.
+
+## Key aspects of suggested implementation
+
+### 1. `movement` pads individuals, not frames
+
+`from_via_tracks_file` sizes the time axis as `n_frames = len(np.unique(frame_numbers))`
+* these are the frames **with data**, not `max - min + 1`. The dense grid it returns is *(unique frames
+present) × (unique individuals present)*:
+- a frame that **no** individual appears in is absent from the time
+  coordinate altogether.
+
+[`write_tracked_detections_to_csv`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/tracker/utils/io.py#L75-L101)
+iterates over the boxes *within* a frame, so a frame with no tracked crabs contributes no rows at
+all. Those frames disappear.
+
+On a 6-frame clip whose VIA tracks file has rows only for frames 0, 1, 3 and 5, with crab 2 also
+missing from frame 1:
+
+```python
+# frame numbers as written in the file:
+use_frame_numbers_from_file=True
+  ds.time.values : [0 1 3 5]             # frames 2 and 4 are absent, not NaN-padded
+     frame 0: [125. 125.]
+     frame 1: [135.  nan]                # crab 2 IS NaN-padded
+     frame 3: [155. 185.]
+     frame 5: [175. 225.]
+
+# the current default:
+use_frame_numbers_from_file=False
+  ds.time.values : [0 1 2 3]             # frame 3's data sits at time 2, frame 5's at time 3
+```
+
+### 2. `escape_state` is written by position, not by frame
+
+[Line 87](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L87)
+takes its length from the data and its index from the metadata:
+
+```python
+escape_state = np.zeros(ds.time.shape[0], dtype=np.float16)   # however many rows survived
+escape_state[local_escape_start_frame_0idx:] = 1.0            # slices frames indices in the clip --- this is the issue
+```
+
+The second line is the issue. The two lines only agree when row *i* is clip frame *i*.
+
+There is a second failure mode. When `local_escape_start_frame_0idx` exceeds the number in `ds.time.shape[0]`,
+the slice assigns nothing and the escape never switches on.
+
+The repository's own test fixture does
+this: it annotates a 101-frame clip that only has 7 frames of data with an escape starting at clip frame 51. So `escape_state`
+is length 7 and `escape_state[51:] = 1.0` is a silent no-op.
+
+### 3. The two changes are jointly necessary, and neither works alone
+
+- **`use_frame_numbers_from_file=True` alone changes nothing.** It relabels the rows; it does not add the missing ones.
+- **The reindex alone is worse than doing nothing.** The current default renumbers the
+  surviving frames `0, 1, 2, …`, so the onset that relies on actual clip frames is wrong. Reindexing onto the clip's range sees those labels as genuine,
+  leaves the shifted data where it is, and appends NaN rows at the end.
+- **Together they work.**
+  * `use_frame_numbers_from_file=True` makes the labels true frame numbers;
+  * reindexing aligns the rows to those labels and fills the holes.
+
+### 4. The clip's frame count comes from the metadata csv
+
+`n_clip_frames` is `loop_END_frame_ffmpeg - loop_START_frame_ffmpeg + 1`, the same expression
+[`extract_loop_clips.py`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/scripts/extract_loop_clips.py#L99-L101)
+uses to check the clips it cuts.
+
+- **The metadata csv is the only source available.** `create-zarr-dataset` is given a directory of
+  VIA tracks files and the metadata csv. It never opens the clip videos.
+- **The metadata csv is already the authority for the escape frame**, which is expressed in global
+  video frames and converted with `loop_START_frame_ffmpeg`. Taking the clip length from the same
+  row keeps one source of truth.
+- **The frame numbers in the VIA tracks file are clip-local**, because
+  [`core_detection_and_tracking`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/tracker/track_video.py#L243-L271)
+  counts `frame_idx` from 0 over the video it is given, and that video is the clip that
+  `extract-loop-clips` cut.
+
+> [!NOTE]
+> `--verify_frames` is opt-in
+> ([`extract_loop_clips.py:388-394`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/scripts/extract_loop_clips.py#L388-L394)),
+> so nothing guarantees a cut clip has exactly the frame count its metadata row states. The guard in
+> step 3 is what makes a disagreement visible.
+
+## Detailed implementation
+
+```mermaid
+flowchart TD
+    L1["load_bboxes.from_via_tracks_file<br/>✨ use_frame_numbers_from_file=True"] --> L2
+    L2["look up the clip's row in the metadata csv"] --> L3
+    L3["✨ n_clip_frames =<br/>loop_END_frame_ffmpeg - loop_START_frame_ffmpeg + 1"] --> L4
+    L4{"✨ frame numbers within<br/>0 … n_clip_frames-1 ?<br/><i>gaps are fine</i>"}
+    L4 -- no --> X1["raise ValueError<br/><i>names the likely cause</i>"]
+    L4 -- yes --> L5["✨ ds.reindex(time=arange(n_clip_frames))"]
+    L5 --> L6{"✨ escape frame in<br/>0 … n_clip_frames-1 ?"}
+    L6 -- no --> X2["raise ValueError"]
+    L6 -- yes --> L7["escape_state = zeros(n_clip_frames)<br/>escape_state[local_escape_start:] = 1.0"]
+    L7 --> L8["expand_dims('clip_id'), clip coords<br/><i>unchanged</i>"]
+    style L1 fill:#e0e7ff,stroke:#4338ca,color:#1e1b4b
+    style L3 fill:#e0e7ff,stroke:#4338ca,color:#1e1b4b
+    style L4 fill:#e0e7ff,stroke:#4338ca,color:#1e1b4b
+    style L5 fill:#e0e7ff,stroke:#4338ca,color:#1e1b4b
+    style L6 fill:#e0e7ff,stroke:#4338ca,color:#1e1b4b
+```
+
+Arrows are the order of statements inside `load_extended_ds` after this change. ✨ marks what this
+pull request adds.
+
+Node → code: every node is inside
+[`load_extended_ds`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L49-L118);
+`L1` replaces [line 70](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L70)
+and `L7` replaces [lines 87-89](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L87-L89).
+
+Note that the "frame numbers within range" check is required because reindex doesn't only fill in gaps — it also drops out-of-range labels. Labels not in the new index are discarded, silently:
+```
+frames 99…199, reindex(time=arange(101))
+  → only labels 99 and 100 match; the other 99 rows are thrown away
+  → a 101-frame dataset that is NaN everywhere except two rows, no error
+
+frames 0…101, reindex(time=arange(101))
+  → frame 101's row is dropped, no warning
+```
+
+### The changes
+
+| # | Change | Diff or signature |
+|---|---|---|
+| 1 | Ask `movement` for the frame numbers as written in the file | `from_via_tracks_file(path, use_frame_numbers_from_file=True)` |
+| 2 | Derive the clip's frame count from the metadata row | `n_clip_frames = global_clip_end_frame_0idx - global_clip_start_frame_0idx + 1` |
+| 3 | **new** guard on the frame numbers in the file | `_validate_frames_in_clip_range(...) -> None` |
+| 4 | Reindex onto the clip's frame range | `ds = ds.reindex(time=np.arange(n_clip_frames))` |
+| 5 | Size `escape_state` by the clip, and guard the escape frame | `np.zeros(n_clip_frames, dtype=np.float16)` |
+| 6 | Assert the values of `escape_state` in the unit tests | see [Tests](#tests) |
+
+<details>
+<summary><b>1-5. <code>load_extended_ds</code> in full</b></summary>
+
+The metadata row is looked up before the frame axis is settled, because the clip's frame count
+comes from that row.
+
+```diff
+-    # Load VIA tracks file as movement dataset
+-    ds = load_bboxes.from_via_tracks_file(via_tracks_file_path)
++    # Load VIA tracks file as movement dataset.
++    # We request the frame numbers as defined in the file. With the
++    # default (use_frame_numbers_from_file=False), `movement` maps them to
++    # a 0-based sequence of *consecutive* integers. Since the tracker
++    # writes no rows for frames with no tracked boxes, that would shift
++    # every subsequent frame earlier, and `time` would no longer be the
++    # clip's frame index.
++    ds = load_bboxes.from_via_tracks_file(
++        via_tracks_file_path, use_frame_numbers_from_file=True
++    )
+
+     # Extract metadata for this row
+     clip_filename = _via_tracks_to_clip_filename(ds.attrs["source_file"])
+     row = df_metadata.loc[df_metadata["loop_clip_name"] == clip_filename].iloc[
+         0
+     ]
+     global_clip_start_frame_0idx = row["loop_START_frame_ffmpeg"] - 1
+     global_clip_end_frame_0idx = row["loop_END_frame_ffmpeg"] - 1
+     global_escape_start_frame_0idx = row["escape_START_frame_0_based_idx"]
++    n_clip_frames = (
++        global_clip_end_frame_0idx - global_clip_start_frame_0idx + 1
++    )
++
++    # Check the frames in the VIA tracks file belong to this clip
++    _validate_frames_in_clip_range(
++        ds.time.values,
++        n_clip_frames,
++        global_clip_start_frame_0idx,
++        clip_filename,
++    )
++
++    # Pad the time coordinate to span the full clip.
++    # `movement` pads with NaNs the individuals that are missing from a
++    # frame, but a frame that no individual appears in is absent from the
++    # time coordinate altogether, so we reindex to add it back.
++    n_frames_wout_boxes = n_clip_frames - ds.sizes["time"]
++    if n_frames_wout_boxes:
++        print(
++            f"{clip_filename}: {n_frames_wout_boxes} of {n_clip_frames} "
++            "frames have no tracked boxes; padding them with NaNs."
++        )
++    ds = ds.reindex(time=np.arange(n_clip_frames))
+
+     # Add escape_state as data variable
+     local_escape_start_frame_0idx = (
+         global_escape_start_frame_0idx - global_clip_start_frame_0idx
+     )
++    if not 0 <= local_escape_start_frame_0idx < n_clip_frames:
++        raise ValueError(
++            f"{clip_filename}: the escape start frame "
++            f"({local_escape_start_frame_0idx} as a clip frame index) is "
++            f"outside the clip's frame range 0-{n_clip_frames - 1}."
++        )
+     # we use float16 (not int/bool) to allow for NaN padding after
+     # concatenating along clip_id
+-    escape_state = np.zeros(ds.time.shape[0], dtype=np.float16)
++    escape_state = np.zeros(n_clip_frames, dtype=np.float16)
+     escape_state[local_escape_start_frame_0idx:] = 1.0
+     ds["escape_state"] = ("time", escape_state)
+```
+
+`print` rather than `logging`, to match the progress output the module already writes at
+[line 242](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L242).
+`np` and `Path` are already imported.
+
+Verified across five patterns of crab-free frames — none, leading, interior, two consecutive interior, and
+trailing. All give `len(time) == n_clip_frames`, the escape onset on the right frame, the crab-free
+frames present as all-NaN rows, and `ds.attrs` preserved through `reindex`.
+
+</details>
+
+<details>
+<summary><b>3. The new guard, and what it separates</b></summary>
+
+The guard sits beside the other `_`-prefixed helpers in the module.
+
+```python
+def _validate_frames_in_clip_range(
+    frames_0idx: np.ndarray,
+    n_clip_frames: int,
+    clip_first_frame_0idx: int,
+    clip_filename: str,
+) -> None:
+    """Check the frame numbers in a VIA tracks file belong to the clip.
+
+    The frame numbers are expected to be 0-based indices into the clip,
+    which is how `detect-and-track-video` numbers the frames of the clip
+    it is given.
+    """
+    if frames_0idx.min() >= 0 and frames_0idx.max() < n_clip_frames:
+        return
+
+    raise ValueError(
+        f"{clip_filename}: the VIA tracks file holds frame numbers "
+        f"{frames_0idx.min()}-{frames_0idx.max()}, outside the clip's "
+        f"frame range 0-{n_clip_frames - 1}. Frame numbers are expected "
+        f"to be 0-based indices into the clip, which starts at video "
+        f"frame {clip_first_frame_0idx}. Either the file was tracked on "
+        f"the full video or paired with the wrong metadata row, or the "
+        f"cut clip does not have the frame count the metadata csv states "
+        f"(re-run extract-loop-clips --verify_frames)."
+    )
+```
+
+One message names both causes, and the numbers in it tell them apart:
+
+| frames in the file | cause | how the numbers show it |
+|---|---|---|
+| `99..199` for a 101-frame clip starting at video frame 99 | tracked on the full video, or paired with the wrong metadata row | the range matches the clip's *global* span, not `0..100` |
+| `0..101` for a 101-frame clip | the cut clip is longer than the metadata states | the range starts at 0 and overshoots the end by a little |
+
+Measured against both cases, and against three clip-based cases (no gaps, interior gaps, trailing
+gaps) which pass.
+
+</details>
+
+<details>
+<summary><b>Rejected alternative — fix <code>escape_state</code> only, and leave the holes</b></summary>
+
+Writing `escape_state` by label rather than by position fixes the onset without any reindex:
+
+```python
+ds["escape_state"] = (ds.time >= local_escape_start_frame_0idx).astype(np.float16)
+```
+
+It is three lines rather than twenty, and it is rejected for three reasons:
+
+1. **It makes the stored time axis depend on which clips are in the job.**
+[`create_final_zarr_store`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L321-L327)
+concatenates the clips of a video with `join="outer"`, which takes the union of their time labels. A clip missing frame 4 keeps that hole only if no sibling clip fills it:
+
+    ```
+    job = {Loop01}         -> time = [0 1 2 3 5]      hole at 4
+    job = {Loop01, Loop03} -> time = [0 1 2 3 4 5]    Loop03 filled it
+    ```
+
+    The current code cannot produce a hole, because it renumbers every clip to `arange(n_present)` and a
+    union of nested ranges is a range. Leaving the holes in would therefore introduce a new problem
+    rather than only failing to fix one.
+
+2. **It leaves `escape_state` undefined at frames where it is perfectly well defined.** The outer join
+NaN-fills every `time`-dimensioned variable, `escape_state` included:
+
+    ```
+    time                : [0 1 2 3 4 5]
+    escape_state Loop01 : [ 0.  0.  0.  1. nan  1.]     escape began at frame 3
+    ```
+
+    That overloads the NaN. The comment at
+    [lines 85-87](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L85-L87)
+    says `float16` is chosen so that clips shorter than the longest clip can be NaN-padded. A mid-clip
+    hole would make "past the end of this clip" and "no crabs were detected here" the same value.
+
+3. **It leaves `len(time)` smaller than the clip.**
+
+    [`00_notebook_data_structure.py:262`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/notebooks/crabs_dataset/00_notebook_data_structure.py#L262)
+    reads `len(position_clip.time)` directly, and
+    [`clip_last_frame_0idx - clip_first_frame_0idx + 1`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L99-L116)
+    would keep disagreeing with it.
+
+</details>
+
+## Files changed
+
+| File | Change |
+|---|---|
+| [`crabs/zarr/create_dataset.py`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py) | `load_extended_ds`: anchor `time` to the clip; **new** `_validate_frames_in_clip_range` |
+| [`tests/test_unit/test_create_zarr_dataset.py`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/tests/test_unit/test_create_zarr_dataset.py) | assert the values of `escape_state` and `time`; cover both guards |
+| [`guides/CreateZarrDatasetForTracks.md`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/guides/CreateZarrDatasetForTracks.md) | state what `time` means, and that existing stores predate the fix |
+
+Nothing else is touched. In particular, no change to `create_temp_zarr_store`,
+`create_final_zarr_store`, `_renumber_individuals`, `DEFAULT_CHUNK_SIZES`, the metadata csv schema,
+or the tracker.
+
+## Tests
+
+Required. The behaviour is currently untested, and the fixture that would have caught it is the one that
+demonstrates the bug.
+
+**Keep the existing fixtures.**
+* `sample_via_tracks_file_factory`
+([`:23-52`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/tests/test_unit/test_create_zarr_dataset.py#L23-L52))
+writes frames `[0, 10, 20, 50, 60, 80, 100]` and
+* `sample_metadata_df_factory`
+([`:55-82`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/tests/test_unit/test_create_zarr_dataset.py#L55-L82))
+describes a 101-frame clip with the escape at clip frame 51. That pairing is the bug's best witness,
+so it must not be tidied into a contiguous range.
+
+Unit tests to add, all in
+[`test_create_zarr_dataset.py`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/tests/test_unit/test_create_zarr_dataset.py):
+
+1. **`escape_state` values, on the existing gappy fixture.** Fails before the change, passes after.
+
+    ```python
+    ds = load_extended_ds(via_tracks_path, df_metadata)
+    escape_state = ds.escape_state.values.ravel()
+    assert ds.sizes["time"] == 101
+    assert np.array_equal(ds.time.values, np.arange(101))
+    assert int(np.argmax(escape_state)) == 51
+    assert escape_state.sum() == 101 - 51
+    ```
+
+2. **Crab-free frames are all-NaN rows.**
+
+    Frames 1 to 9 hold no boxes in the fixture, so
+   `ds.position.isel(time=1).isnull().all()`.
+
+3. **A contiguous VIA tracks file is unaffected.**
+
+    One row per frame of the clip gives the same
+   `escape_state` as test 1, and no NaN rows.
+
+4. **The frame-range guard fires on out-of-range frame numbers.**
+
+    Parametrized over both causes: a file whose frames run `99 … 199` (video-based numbering)
+   and one whose frames run `0 … 101` (cut clip longer than the metadata states). Both raise
+   `ValueError`. Assert that the message reports the file's own range and the clip's
+   `0-100` — the numbers, not the wording.
+
+5. **The escape-frame guard fires.**
+
+    A metadata row whose `escape_START_frame_0_based_idx` falls
+   outside the clip raises `ValueError`. Today the slice silently assigns nothing.
+
+Integration: none added.
+[`test_create_temp_zarr_store`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/tests/test_unit/test_create_zarr_dataset.py#L205)
+and
+[`test_create_final_zarr_store`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/tests/test_unit/test_create_zarr_dataset.py#L269)
+already exercise the path end to end and must stay green with the same fixtures.
+
+## Documentation updates
+
+Yes, one file.
+[`guides/CreateZarrDatasetForTracks.md`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/guides/CreateZarrDatasetForTracks.md)
+gains two lines under *Expected output*:
+
+- `time` is the clip's 0-based frame index and always spans the whole clip, with frames that hold no
+  tracked crabs stored as NaNs.
+- add a suggestion for how to get the full range of a clip (without trailing nan-padding, but with any NaN frames that are inside the clip range). E.g.
+
+  ```python
+  # Preferred option to get a clip's full range without trailing NaN-padding
+  d = ds_video.isel(clip_id=0)
+  n_clip_frames = int(d.clip_last_frame_0idx - d.clip_first_frame_0idx) + 1
+  d_clip = d.isel(time=slice(0, n_clip_frames))   # no dropna, no NaN archaeology
+  ```
+
+- stores built before this change have a `time` coordinate that does not mean this, and should be
+  rebuilt if the audit below finds affected clips.
+
+## Verifications for agent to run
+
+```bash
+# the strengthened test must fail before the change and pass after
+pytest tests/test_unit/test_create_zarr_dataset.py -v
+
+pytest tests/test_unit
+pytest tests/test_integration # run at the end, slower
+pre-commit run --all-files
+```
+
+
+**Rebuild a small store and check the invariant**, on two or three clips:
+
+```bash
+create-zarr-dataset \
+    --via_tracks_dir <dir> --metadata_csv <loop-frames-ffmpeg.csv> \
+    --zarr_store /tmp/check.zarr --zarr_mode_store w
+```
+
+```python
+import xarray as xr, numpy as np
+ds = xr.open_datatree("/tmp/check.zarr", engine="zarr", chunks={})["<video-id>"].to_dataset()
+for c in ds.clip_id.values:
+    d = ds.sel(clip_id=c)
+    expected = int(d.clip_escape_first_frame_0idx - d.clip_first_frame_0idx)
+    assert int(np.argmax(d.escape_state.values)) == expected, c
+```
+
+## Points to discuss
+
+| # | To discuss | Conclusion |
+|---|---|---|
+| 1 | **Pad the time axis, or only fix `escape_state`?** Writing `escape_state` by label is a three-line fix that corrects the onset but leaves holes in `time`. Padding is larger and also makes the axis a stated property of the clip. **I recommend padding**, for the three reasons in *Rejected alternative*. | We do padding |
+| 2 | **Are existing stores affected?** Unknown until the audit runs — `/ceph` was not reachable while writing this. With ~100 crabs per frame, `min_hits: 1` and `max_age: 10`, a crab-free frame should be rare; but the source directory is named `…above_10th_percentile…`, which suggests sparse loops are deliberately included. **Run the audit before merging**, so the guide can say whether a rebuild is needed. | I will run the audit separately and regenerate the zarr store in any case after this is merged. The guide does not need to signal where a rebuilt is needed, just that it may be if generated prior to this change. |
+| 3 | **Should `movement` change too?** `from_via_tracks_file` can return a non-uniformly sampled `time` axis, and movement's own kinematics handle that inconsistently — `compute_forward_displacement` uses positional `.diff`, `compute_velocity` uses coordinate-aware `.differentiate`, so on a gapped axis they disagree about what one time step is. **I recommend filing an upstream issue** proposing that `use_frame_numbers_from_file=True` reindex to `arange(min, max+1)`. It would not remove the reindex here, because only the metadata csv knows the clip's length. Not a blocker. | I opened a PR in movement that does not fill gaps in time (in line with their current treatment of pose data), but it retains any gaps in the original VIA tracks file frame numbers if `use_frame_numbers_from_file=True`. See PR [1103](https://github.com/neuroinformatics-unit/movement/pull/1103)  |
+| 4 | **`output_video.py:176` has the same hazard.** It reads a `_tracks.csv` with `use_frame_numbers_from_file=False` and draws the boxes onto video frames, so a crab-free frame would shift the overlay. Out of scope here; worth its own issue. | Fix implemented as part of this PR |
+| 5 | **`_renumber_individuals` discards the original track IDs** ([lines 193-200](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L193-L200)), resetting each clip's individuals to `id_0 … id_{N-1}`. That makes the store unusable as a source of tracker track IDs. Deliberate for the escape analysis, but worth recording. Out of scope. | We don't care about retaining the exact ID returned by the tracker. We just care about them being "distinct" as the tracker defined them (regardless of their exact name). We force them to be consecutive and monotonically increasing to keep the `individual` axis compact in the zarr store. So that can be left as is.|

From 445f33c862b7bffe977ab5af7e261e046f065f4c Mon Sep 17 00:00:00 2001
From: sfmig <33267254+sfmig@users.noreply.github.com>
Date: Wed, 16 Sep 2026 13:47:30 +0100
Subject: [PATCH 05/19] Final reviewed proposal

---
 claude-plans/proposal-zarr-time-coordinate.md | 138 ++++++++++++++----
 1 file changed, 107 insertions(+), 31 deletions(-)

diff --git a/claude-plans/proposal-zarr-time-coordinate.md b/claude-plans/proposal-zarr-time-coordinate.md
index cd78985c..9353d90a 100644
--- a/claude-plans/proposal-zarr-time-coordinate.md
+++ b/claude-plans/proposal-zarr-time-coordinate.md
@@ -3,9 +3,20 @@
 ## Description
 
 The `time` coordinate of the tracks zarr dataset is not the clip's frame index, so `escape_state`
-marks the escape on the wrong frame. This proposal anchors `time` to the clip, in
+marks the escape on the wrong frame. This proposal makes the `time` axis dense, in
 [`load_extended_ds`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L49).
 
+Two things change, and they are worth keeping apart.
+* `time` becomes the clip's **frame index**, and
+* `time` becomes **dense**.
+
+As a result, the axis is uniformly sampled and a frame
+with no detections is a NaN row rather than an absent time coordinate.
+
+Only the first is needed to fix the bug in `escape_state` (although it could also be fixed differently). The second is the larger claim, and it is argued in [§5](#5-why-time-should-be-dense) — it is the
+reason the *Rejected alternative* under [Detailed implementation](#detailed-implementation) is
+rejected.
+
 ```mermaid
 flowchart TD
     A["&lt;clip&gt;_tracks.csv<br/><i>rows only for frames with tracked crabs</i>"] --> B
@@ -49,7 +60,6 @@ Node → code:
 4. Reindex the dataset onto the clip's full frame range.
 5. Build `escape_state` over that frame range, and raise if the escape frame falls outside it.
 6. Strengthen `test_load_extended_ds` to assert the values of `escape_state`, not just its presence.
-7. Audit the VIA tracks files on the cluster to decide whether existing stores must be rebuilt.
 
 ## Key aspects of suggested implementation
 
@@ -92,7 +102,11 @@ escape_state = np.zeros(ds.time.shape[0], dtype=np.float16)   # however many row
 escape_state[local_escape_start_frame_0idx:] = 1.0            # slices frames indices in the clip --- this is the issue
 ```
 
-The second line is the issue. The two lines only agree when row *i* is clip frame *i*.
+The second line is the issue. The two lines only agree when row *i* is clip frame *i*. It could be fixed as:
+```python
+ds["escape_state"] = (ds.time >= local_escape_start_frame_0idx).astype(np.float16)
+```
+but that requires remembering that we need to use time coordinates for lookup, rather than time indices.
 
 There is a second failure mode. When `local_escape_start_frame_0idx` exceeds the number in `ds.time.shape[0]`,
 the slice assigns nothing and the escape never switches on.
@@ -101,16 +115,22 @@ The repository's own test fixture does
 this: it annotates a 101-frame clip that only has 7 frames of data with an escape starting at clip frame 51. So `escape_state`
 is length 7 and `escape_state[51:] = 1.0` is a silent no-op.
 
-### 3. The two changes are jointly necessary, and neither works alone
+### 3. To make `time` dense, the two changes are jointly necessary
 
 - **`use_frame_numbers_from_file=True` alone changes nothing.** It relabels the rows; it does not add the missing ones.
 - **The reindex alone is worse than doing nothing.** The current default renumbers the
-  surviving frames `0, 1, 2, …`, so the onset that relies on actual clip frames is wrong. Reindexing onto the clip's range sees those labels as genuine,
-  leaves the shifted data where it is, and appends NaN rows at the end.
+  surviving frames `0, 1, 2, …`,. Reindexing onto the clip's range sees those labels as genuine,
+  leaves the shifted escape data where it is, and appends NaN rows at the end.
 - **Together they work.**
   * `use_frame_numbers_from_file=True` makes the labels true frame numbers;
   * reindexing aligns the rows to those labels and fills the holes.
 
+> [!NOTE]
+> This is about making the axis **dense**, not about the bug. The `escape_state` onset is fixed by
+> `use_frame_numbers_from_file=True` plus writing `escape_state` by label, with no reindex at all
+> (see *Rejected alternative* under [Detailed implementation](#detailed-implementation)). Whether to
+> also pad is a separate decision, argued in [§5](#5-why-time-should-be-dense).
+
 ### 4. The clip's frame count comes from the metadata csv
 
 `n_clip_frames` is `loop_END_frame_ffmpeg - loop_START_frame_ffmpeg + 1`, the same expression
@@ -133,6 +153,35 @@ uses to check the clips it cuts.
 > so nothing guarantees a cut clip has exactly the frame count its metadata row states. The guard in
 > step 3 is what makes a disagreement visible.
 
+### 5. Why should `time` be dense?
+
+This is the case for padding (note that the `escape_state` bug is fixable without
+any reindex).
+
+1. **A gapped `time` is not uniformly sampled, and no user convention repairs that.** Telling people to
+slice with `.sel` rather than `.isel` fixes *lookup*; it does nothing for anything that walks the
+axis, e.g. `movement`'s own kinematics:
+
+    * `compute_forward_displacement` uses positional `.diff`, so a two-frame gap counts as one step,
+    * `compute_velocity` uses coordinate-aware `.differentiate`.
+    * Rolling windows,
+    * `n_frames / fps`
+    durations, and any speed trace around the escape onset are then silently wrong.
+
+    On a dense axis the
+    same query returns NaN for a frame with no crabs, which is the honest answer. The user does not need to keep in mind the fact that there are "missing" frames and assess whether they may have an impact or not.
+
+2. **It makes "missing" mean one thing.** `movement` already NaN-pads an individual that is absent from
+a frame (§1). A frame that *every* individual is absent from is the same fact, and dropping its label
+rather than NaN-filling it records that fact differently depending only on how many crabs happened to
+be visible. Reindexing makes the rule uniform: missing is always a NaN row, never an absent label. Less cognitive load for the user.
+
+3. The storage cost is nil.
+[`create_final_zarr_store`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L321-L327)
+already concatenates with `join="outer"`, so the stored array is dense to the *union* of the clips'
+time labels. Padding only changes that length from "frames some clip in this video had a crab" to
+"the longest clip in this video" — a handful of rows.
+
 ## Detailed implementation
 
 ```mermaid
@@ -162,7 +211,7 @@ Node → code: every node is inside
 `L1` replaces [line 70](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L70)
 and `L7` replaces [lines 87-89](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L87-L89).
 
-Note that the "frame numbers within range" check is required because reindex doesn't only fill in gaps — it also drops out-of-range labels. Labels not in the new index are discarded, silently:
+Note that the "frame numbers within range" check is required because **reindex doesn't only fill in gaps — it also drops out-of-range labels**. Labels not in the new index are discarded, silently:
 ```
 frames 99…199, reindex(time=arange(101))
   → only labels 99 and 100 match; the other 99 rows are thrown away
@@ -172,13 +221,13 @@ frames 0…101, reindex(time=arange(101))
   → frame 101's row is dropped, no warning
 ```
 
-### The changes
+### Main changes
 
 | # | Change | Diff or signature |
 |---|---|---|
 | 1 | Ask `movement` for the frame numbers as written in the file | `from_via_tracks_file(path, use_frame_numbers_from_file=True)` |
 | 2 | Derive the clip's frame count from the metadata row | `n_clip_frames = global_clip_end_frame_0idx - global_clip_start_frame_0idx + 1` |
-| 3 | **new** guard on the frame numbers in the file | `_validate_frames_in_clip_range(...) -> None` |
+| 3 | **New** guard on the frame numbers in the file | `_validate_frames_in_clip_range(...) -> None` |
 | 4 | Reindex onto the clip's frame range | `ds = ds.reindex(time=np.arange(n_clip_frames))` |
 | 5 | Size `escape_state` by the clip, and guard the escape frame | `np.zeros(n_clip_frames, dtype=np.float16)` |
 | 6 | Assert the values of `escape_state` in the unit tests | see [Tests](#tests) |
@@ -317,22 +366,34 @@ Writing `escape_state` by label rather than by position fixes the onset without
 ds["escape_state"] = (ds.time >= local_escape_start_frame_0idx).astype(np.float16)
 ```
 
-It is three lines rather than twenty, and it is rejected for three reasons:
+**It does fix the reported bug.** The onset lands on the right frame. It is rejected only because it leaves `time` non-uniformly
+sampled — [§5](#5-why-time-should-be-dense) is the whole of the case against it.
 
-1. **It makes the stored time axis depend on which clips are in the job.**
+It is also a smaller diff, though less so than it looks: the substantive difference is
+one line (`ds.reindex(...)`) against one line (the label comparison). Most of the rest is
+`_validate_frames_in_clip_range`, which is worth having either way — a file tracked on the full
+video, or paired with the wrong metadata row, should be an error rather than a dataset that quietly
+looks fine.
+
+Three further differences, none decisive on its own:
+
+1. **The stored time axis becomes emergent rather than stated.**
 [`create_final_zarr_store`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L321-L327)
 concatenates the clips of a video with `join="outer"`, which takes the union of their time labels. A clip missing frame 4 keeps that hole only if no sibling clip fills it:
 
     ```
-    job = {Loop01}         -> time = [0 1 2 3 5]      hole at 4
-    job = {Loop01, Loop03} -> time = [0 1 2 3 4 5]    Loop03 filled it
+    Loop01 alone        -> time = [0 1 2 3 5]      hole at 4
+    Loop01 with Loop03  -> time = [0 1 2 3 4 5]    Loop03 filled it
     ```
 
-    The current code cannot produce a hole, because it renumbers every clip to `arange(n_present)` and a
-    union of nested ranges is a range. Leaving the holes in would therefore introduce a new problem
-    rather than only failing to fix one.
+    Each array job processes one whole video, so this is deterministic for a given input directory —
+    not run-to-run flakiness. But `ds_video.time` then means "frames where some clip in this video
+    had a crab", which is a property of the detections rather than of the clips. Reindexing makes it
+    `arange(max n_clip_frames)`, which is a property of the clips. The current code cannot produce a
+    hole at all, because it renumbers every clip to `arange(n_present)` and a union of nested ranges
+    is a range.
 
-2. **It leaves `escape_state` undefined at frames where it is perfectly well defined.** The outer join
+2. **It leaves `escape_state` NaN at frames where it is perfectly well defined.** The outer join
 NaN-fills every `time`-dimensioned variable, `escape_state` included:
 
     ```
@@ -344,13 +405,25 @@ NaN-fills every `time`-dimensioned variable, `escape_state` included:
     [lines 85-87](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L85-L87)
     says `float16` is chosen so that clips shorter than the longest clip can be NaN-padded. A mid-clip
     hole would make "past the end of this clip" and "no crabs were detected here" the same value.
-
-3. **It leaves `len(time)` smaller than the clip.**
-
-    [`00_notebook_data_structure.py:262`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/notebooks/crabs_dataset/00_notebook_data_structure.py#L262)
-    reads `len(position_clip.time)` directly, and
-    [`clip_last_frame_0idx - clip_first_frame_0idx + 1`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L99-L116)
-    would keep disagreeing with it.
+    Recoverable rather than lost, though: `clip_first_frame_0idx` and `clip_last_frame_0idx` are
+    stored per `clip_id`, so the two cases can always be told apart, and `escape_state` is derivable
+    from those coordinates in the first place. An annoyance, not a reason.
+
+3. **Positional indexing stops agreeing with frame numbers.** On a dense axis `isel(time=i)` and
+`sel(time=i)` are the same row, so existing code, and third-party code that indexes positionally,
+stay correct without anyone learning a convention. On a gapped axis `.isel` still runs and returns a
+plausible wrong row, and nothing detects the violation. The recipe under
+[Documentation updates](#documentation-updates) is an example: `isel(time=slice(0, n_clip_frames))`
+needs row *i* to be frame *i*; the gapped equivalent `sel(time=slice(0, n_clip_frames - 1))` runs
+fine but hands back fewer rows than the clip has frames.
+
+    This is convenience, not a checked guarantee, and it is **not** an argument that
+    `dropna(dim="time", how="all")` behaves better under padding — it does not. `dropna` cannot tell
+    "beyond this clip's end" from "no crabs in this frame", so under *both* designs it returns the
+    clip's frames *with detections*, and the assertion at
+    [`00_notebook_data_structure.py:261-266`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/notebooks/crabs_dataset/00_notebook_data_structure.py#L261-L266)
+    fails as soon as one frame is crab-free. That idiom needs replacing with the slice recipe either
+    way.
 
 </details>
 
@@ -439,8 +512,12 @@ gains two lines under *Expected output*:
   d_clip = d.isel(time=slice(0, n_clip_frames))   # no dropna, no NaN archaeology
   ```
 
-- stores built before this change have a `time` coordinate that does not mean this, and should be
-  rebuilt if the audit below finds affected clips.
+  and say why not `dropna(dim="time", how="all")`: it cannot tell "beyond this clip's end" from "no
+  crabs in this frame", so it silently drops crab-free frames from the middle of the clip and
+  returns a gapped axis. The slice keeps every frame of the clip and nothing else.
+
+- stores built before this change have a `time` coordinate that does not mean this, and may need
+  rebuilding.
 
 ## Verifications for agent to run
 
@@ -475,8 +552,7 @@ for c in ds.clip_id.values:
 
 | # | To discuss | Conclusion |
 |---|---|---|
-| 1 | **Pad the time axis, or only fix `escape_state`?** Writing `escape_state` by label is a three-line fix that corrects the onset but leaves holes in `time`. Padding is larger and also makes the axis a stated property of the clip. **I recommend padding**, for the three reasons in *Rejected alternative*. | We do padding |
-| 2 | **Are existing stores affected?** Unknown until the audit runs — `/ceph` was not reachable while writing this. With ~100 crabs per frame, `min_hits: 1` and `max_age: 10`, a crab-free frame should be rare; but the source directory is named `…above_10th_percentile…`, which suggests sparse loops are deliberately included. **Run the audit before merging**, so the guide can say whether a rebuild is needed. | I will run the audit separately and regenerate the zarr store in any case after this is merged. The guide does not need to signal where a rebuilt is needed, just that it may be if generated prior to this change. |
-| 3 | **Should `movement` change too?** `from_via_tracks_file` can return a non-uniformly sampled `time` axis, and movement's own kinematics handle that inconsistently — `compute_forward_displacement` uses positional `.diff`, `compute_velocity` uses coordinate-aware `.differentiate`, so on a gapped axis they disagree about what one time step is. **I recommend filing an upstream issue** proposing that `use_frame_numbers_from_file=True` reindex to `arange(min, max+1)`. It would not remove the reindex here, because only the metadata csv knows the clip's length. Not a blocker. | I opened a PR in movement that does not fill gaps in time (in line with their current treatment of pose data), but it retains any gaps in the original VIA tracks file frame numbers if `use_frame_numbers_from_file=True`. See PR [1103](https://github.com/neuroinformatics-unit/movement/pull/1103)  |
-| 4 | **`output_video.py:176` has the same hazard.** It reads a `_tracks.csv` with `use_frame_numbers_from_file=False` and draws the boxes onto video frames, so a crab-free frame would shift the overlay. Out of scope here; worth its own issue. | Fix implemented as part of this PR |
-| 5 | **`_renumber_individuals` discards the original track IDs** ([lines 193-200](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L193-L200)), resetting each clip's individuals to `id_0 … id_{N-1}`. That makes the store unusable as a source of tracker track IDs. Deliberate for the escape analysis, but worth recording. Out of scope. | We don't care about retaining the exact ID returned by the tracker. We just care about them being "distinct" as the tracker defined them (regardless of their exact name). We force them to be consecutive and monotonically increasing to keep the `individual` axis compact in the zarr store. So that can be left as is.|
+| 1 | **Pad the time axis, or only fix `escape_state`?** Writing `escape_state` by label corrects the onset on its own, and leaves the holes in `time`. **I recommend padding**, for the two reasons in [§5](#5-why-time-should-be-dense): a gapped axis is not uniformly sampled, so kinematics over it are silently wrong, and it records a missing frame differently from a missing individual. The three points under *Rejected alternative* are supporting, not decisive. This is worth deciding on its own terms — it is not needed to fix the bug. | We do padding |
+| 2 | **Should `movement` change too?** `from_via_tracks_file` can return a non-uniformly sampled `time` axis, and movement's own kinematics handle that inconsistently — `compute_forward_displacement` uses positional `.diff`, `compute_velocity` uses coordinate-aware `.differentiate`, so on a gapped axis they disagree about what one time step is. **I recommend filing an upstream issue** proposing that `use_frame_numbers_from_file=True` reindex to `arange(min, max+1)`. It would not remove the reindex here, because only the metadata csv knows the clip's length. Not a blocker. | I opened a PR in movement that does not fill gaps in time (in line with their current treatment of pose data), but it retains any gaps in the original VIA tracks file frame numbers if `use_frame_numbers_from_file=True`. See PR [1103](https://github.com/neuroinformatics-unit/movement/pull/1103)  |
+| 3 | **`output_video.py:176` has the same hazard.** It reads a `_tracks.csv` with `use_frame_numbers_from_file=False` and draws the boxes onto video frames, so a crab-free frame would shift the overlay. Out of scope here; worth its own issue. | Fix implemented as part of this PR |
+| 4 | **`_renumber_individuals` discards the original track IDs** ([lines 193-200](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L193-L200)), resetting each clip's individuals to `id_0 … id_{N-1}`. That makes the store unusable as a source of tracker track IDs. Deliberate for the escape analysis, but worth recording. Out of scope. | We don't care about retaining the exact ID returned by the tracker. We just care about them being "distinct" as the tracker defined them (regardless of their exact name). We force them to be consecutive and monotonically increasing to keep the `individual` axis compact in the zarr store. So that can be left as is.|

From a8a847491190dcbb1b2bbae7383dd0b6f3582837 Mon Sep 17 00:00:00 2001
From: sfmig <33267254+sfmig@users.noreply.github.com>
Date: Wed, 16 Sep 2026 13:51:12 +0100
Subject: [PATCH 06/19] Remove sam2 plan

---
 .../proposal-detect-and-track-mask.md         | 826 ------------------
 1 file changed, 826 deletions(-)
 delete mode 100644 claude-plans/proposal-detect-and-track-mask.md

diff --git a/claude-plans/proposal-detect-and-track-mask.md b/claude-plans/proposal-detect-and-track-mask.md
deleted file mode 100644
index 45c1d03d..00000000
--- a/claude-plans/proposal-detect-and-track-mask.md
+++ /dev/null
@@ -1,826 +0,0 @@
-# Proposal for a `detect-and-track-mask` entry point
-
-## Description
-
-This document proposes a **new CLI entry point, `detect-and-track-mask`** in the crabs backage.
-
-This entry point runs the existing
-`detect-and-track-video` pipeline unchanged, then prompts SAM2 with the **tracked** boxes and
-writes one boolean mask per crab per frame, each stored in a plane indexed by that crab's
-**track ID**.
-
-
-> [!NOTE]
-> Nomenclature clarifications
-> * **Prompt** — a hint telling SAM2 *which* object to segment. Here, one bounding box in
-> `[x1, y1, x2, y2]` pixel coordinates per tracked crab.
->
-> * **Instance plane** — a 2-D boolean array holding exactly one crab's mask. The store is a stack
-> of these, indexed by track ID. This is what is written to disk.
-> * **Label image** — a 2-D integer array where `0` is background and every other value identifies
-> one object. This is the array `skimage.measure.regionprops` takes. Here it is **derived on read**
-> from the instance planes (§3), never stored.
-
-
-## References
-It is a scoped-down
-first slice of Phase 3 of
-[`this-repo-collects-tools-pure-kernighan.md`](this-repo-collects-tools-pure-kernighan.md).
-
----
-
-## Key aspects of suggested implementation
-
-
-
-### 1. A subclass, so the diff is one new file
-
-* `Tracking.detect_and_track_video` already produces exactly what SAM2 needs — tracked boxes and
-IDs per frame.
-* It already re-loops over the video twice (for `--save_video` and
-`--save_frames`). Masking is a third such loop.
-
-```mermaid
-flowchart TD
-    subgraph existing["detect-and-track-video (unchanged)"]
-        A["Tracking.detect_and_track_video()"] --> B["core_detection_and_tracking()<br/>loop 1: detect + SORT"]
-        B --> C["write_tracked_detections_to_csv<br/>&lt;video&gt;_tracks.csv"]
-        B --> D["generate_tracked_video<br/>loop 2 (optional)"]
-        B --> E["write_all_video_frames_as_images<br/>loop 3 (optional)"]
-    end
-    subgraph new["✨ detect-and-track-mask"]
-        F["TrackingAndMasking(Tracking)"] -.->|inherits, calls| A
-        F --> G["✨ generate_masks()<br/>loop 4: SAM2 per frame"]
-        G --> H["✨ &lt;video&gt;_masks.zarr"]
-    end
-    style F fill:#e0e7ff,stroke:#4338ca,color:#1e1b4b
-    style G fill:#e0e7ff,stroke:#4338ca,color:#1e1b4b
-    style H fill:#e0e7ff,stroke:#4338ca,color:#1e1b4b
-```
-
-* The **entire feature is two new modules**: `crabs/tracker/track_and_mask_video.py` (the write
-path) and `crabs/tracker/utils/masks.py` (the read-side helper, §3). The second exists only
-because the overlap decision moved to read time; it imports neither `sam2` nor torch.
-
-* There are **two changes to existing Python**, both in
-[`crabs/tracker/track_video.py`](../crabs/tracker/track_video.py):
-    - one added line, so `generate_masks`in `crabs/tracker/track_and_mask_video.py` can see the boxes:
-
-    ```diff
-    --- a/crabs/tracker/track_video.py
-    +++ b/crabs/tracker/track_video.py
-    @@ def detect_and_track_video(self) -> None:
-        # Run detection and tracking over all frames in video
-        tracked_bboxes_dict = self.core_detection_and_tracking()
-    +   self.tracked_bboxes_dict = tracked_bboxes_dict
-    ```
-    - splitting `tracking_parse_args` into a parser builder and the call to `parse_args`, so the
-    new entry point can inherit the tracking arguments with `parents=[...]` (§5). Behaviour of
-    `detect-and-track-video` is unchanged.
-
-* `prep_outputs` in `generate_masks` needs no change:
-    - `generate_masks`  can derive its output path from `self.tracking_output_dir` and `self.input_video_file_root`, in the same way as it does already.
-
-* The masks are
-the third optional artefact in the same directory as the video `--save_video` writes and the raw PNGs `--save_frames` writes.
-    ```
-    tracking_output_<timestamp>/
-    ├── <video>_tracks.csv
-    ├── <video>_tracks.mp4          # --save_video
-    ├── <video>_frames/             # --save_frames
-    └── <video>_masks.zarr          # ✨ always, for this entry point
-    ```
-
-### 2. Output format: a zarr `(T, M, H, W)` boolean array, one plane per track ID
-
-```
-<video>_masks.zarr/                  # zarr group, matching the existing store layout
-└── masks   (T, M, H, W) bool        # dims: image_id, id, img_h, img_w
-```
-
-* Axis 1 is indexed by track ID, with `m = track_id - 1`. That mapping is exact, not a convention:
-`KalmanBoxTracker` numbers itself from a 0-based class counter
-([sort.py:76-77](../crabs/tracker/sort.py#L76-L77)) and `Sort.update` emits `trk.id + 1`
-([sort.py:210](../crabs/tracker/sort.py#L210)), so the IDs in `<video>_tracks.csv` are 1-based.
-
-    ```python
-    import zarr
-
-    masks = zarr.open("<video>_masks.zarr", mode="r")["masks"]
-    masks[t, track_id - 1]     # (H, W) bool — one crab in one frame, SAM2's output verbatim
-    masks[t]                   # (M, H, W)   — every crab in frame t
-    masks[:, track_id - 1]     # (T, H, W)   — one crab's whole trajectory, in a single slice
-    ```
-
-* **Why per-instance planes and not a single label image.** A label image cannot represent two
-crabs occupying the same pixel, and this scene has ~100 frequently touching individuals — so
-flattening would silently discard one crab's overlapping pixels. Here each crab owns its plane:
-overlap is representable, nothing is discarded, and there is **no overlap policy to choose**. The
-ellipse fits Phase 3 is aiming at are therefore not biased by neighbours.
-
-* **It is the shape the existing consumer already builds by hand.**
-[`notebook_visualise_masks_from_zarr.py`](../notebooks/notebook_visualise_masks_from_zarr.py)
-takes a `(T, H, W)` label image and one-hot expands it into exactly
-`(image_id, id, img_h, img_w)` boolean with a dask `map_blocks`
-([:142-174](../notebooks/notebook_visualise_masks_from_zarr.py#L142-L174)), under the comment
-*"CONS: can be slow to compute"*. Writing the planes directly deletes that expansion and its cost,
-and `sel(image_id=..., id=...)`
-([:181](../notebooks/notebook_visualise_masks_from_zarr.py#L181)) keeps working unchanged. The dim
-names above are taken from that notebook.
-
-* **`bool`, and the whole dtype question disappears.** Identity now lives in the *axis*, not in the
-pixel values, so there is nothing to encode and no `int16`/`int32`/`uint16` ceiling to argue about
-— the `100 × n_frames` bound on track IDs no longer constrains the format at all. This also
-disarms the trap in the existing script rather than merely guarding against it:
-
-> [!NOTE]
-> [`generate_masks_from_bboxes.py`](../scripts/generate_masks_from_bboxes.py) opens its store as
-> `dtype="bool"` ([:120](../scripts/generate_masks_from_bboxes.py#L120)) but writes an `int16`
-> ID-encoded array into it ([:191](../scripts/generate_masks_from_bboxes.py#L191)), so every ID is
-> silently coerced to `True` and the instance IDs are lost. Under this format `bool` is the
-> *correct* dtype, because no ID is ever stored in a pixel. The bug cannot be expressed.
-
-* **`M` is known before the masking loop starts**, so the array is never resized: §1 runs detection
-and tracking to completion first, so `M = max(track_id)` over `self.tracked_bboxes_dict`. Not
-every ID in `1..M` is necessarily emitted — a tracker suppressed below `min_hits` still burns a
-counter value — so a few planes stay empty for the whole video. Those are never written.
-
-* **Chunking, and the one real cost of this shape.** `chunks=(1, 1, H, W)` — one chunk per
-(frame, crab), the unit every access pattern above reads. A chunk is 2.07 MB raw at 1920×1080 and
-~0.17% non-zero, so it compresses to a few KB, and zarr does not write all-`False` chunks at all.
-The count is the problem: ~`n_frames × n_crabs` ≈ **300,000 chunks** for a 3000-frame clip, which
-means 300,000 files in a directory store — genuinely bad on the cluster's network filesystem. The
-mitigation is zarr 3's **sharding codec**, `shards=(1, 128, H, W)`, packing 128 planes per file and
-cutting that to `n_frames × ceil(M/128)`. The writer emits a whole frame at a time, so each shard
-is filled in one pass, and `zarr` is declared as `>=3` for it (§6). See *Points to discuss*.
-
-* Metadata in `.attrs` on the group:
-
-```python
-{
-    "timestamp": ..., "sam2_model": ..., "device": ...,
-    "source_video": str(self.input_video_path),
-    "tracks_csv": str(self.csv_file_path),      # the file these IDs refer to
-    "n_frames": ..., "image_shape": [H, W],
-    "dims": ["image_id", "id", "img_h", "img_w"],
-    "mask_encoding": "instance_planes",          # asserted against the array dtype (bool)
-    "track_id_offset": 1,                        # m = track_id - track_id_offset
-    "n_track_ids": ...,                          # M, the size of axis 1
-    "n_track_ids_emitted": ...,                  # planes non-empty in at least one frame
-    "kalman_box_tracker_count": ...,             # KalmanBoxTracker.count, for cross-checking
-    "prompt_type": "bounding_box",
-    "prompt_source": "tracked_boxes",            # not raw detections — see §4
-    "multimask_output": False,
-}
-```
-
-There is deliberately **no `overlap_policy` and no `background_label`** key: this format has
-neither, which is the point of it.
-
-### 3. Writing planes, and rebuilding a label image on read
-
-SAM2 already returns per-instance masks, so the write path has **no combining step at all** — the
-masks go straight into their planes. The flattening that `regionprops` needs happens on the read
-side, where the caller chooses the policy and nothing on disk is affected.
-
-```mermaid
-flowchart LR
-    A["frame (H, W, 3) BGR"] --> B["cvtColor → RGB"]
-    B --> C["predictor.set_image<br/>encoder, once per frame"]
-    D["tracked_boxes (N, 4)<br/>ids (N,)"] --> E
-    C --> E["predictor.predict(box=chunk)<br/>decoder, per chunk of 32"]
-    E --> F["(N, H, W) bool"]
-    F -->|"✨ oindex[t, ids-1]"| I[("&lt;video&gt;_masks.zarr<br/>(T, M, H, W) bool")]
-    I -.->|"read side"| J["✨ to_label_image(store, t)"]
-    J -.-> K["(H, W) int32<br/>for regionprops / napari"]
-    style I fill:#e0e7ff,stroke:#4338ca,color:#1e1b4b
-    style J fill:#e0e7ff,stroke:#4338ca,color:#1e1b4b
-```
-
-**Write side** — one orthogonal-index assignment per chunk of prompts, no helper needed:
-
-```python
-mask_zarr.oindex[frame_idx, track_ids - 1] = masks   # (N, H, W) bool into N planes
-```
-
-`oindex` rather than plain `[...]` because zarr implements vectorised integer indexing through
-orthogonal indexing; `track_ids` are scattered along axis 1, not contiguous.
-
-**Read side** — one helper in a new `crabs/tracker/utils/masks.py`, pure (no `sam2`, no torch), so
-it is unit-testable on CI:
-
-```python
-def to_label_image(mask_planes, track_ids=None, policy="higher_track_id_wins"):
-    """(M, H, W) bool -> (H, W) int32 label image, for regionprops and napari.
-
-    This is where the overlap decision now lives: it is the caller's, per call,
-    and it never touches what is stored.
-    """
-```
-
-`to_label_image` is the old `paint_masks_as_label_image` moved to the read side and given a
-`policy` argument. `ellipses_from_labels` in the notebook
-([:52](../notebooks/notebook_visualise_masks_from_zarr.py#L52)) consumes its output unchanged.
-
-**What this costs.** The store is no longer directly openable in napari — `viewer.add_labels` on a
-`(T, M, H, W)` boolean array gives a slider over crabs, not a frame view. Callers go through
-`to_label_image` instead, which is one line at the two call sites in the notebook
-([:219](../notebooks/notebook_visualise_masks_from_zarr.py#L219),
-[:226-229](../notebooks/notebook_visualise_masks_from_zarr.py#L226-L229)). That is the honest
-trade for losslessness; the notebook change is listed in *Files changed*.
-
-### 4. Prompting with tracked boxes: what you get, and what it costs
-
-Per the request, the prompts are the **tracked** boxes. That is what makes the plumbing trivial —
-the mask rows and the CSV rows are the same rows, so there is no index bookkeeping at all, and no
-change to `sort.py`.
-
-Two consequences worth stating plainly, neither of which blocks this:
-
-- **SORT does not return the detector's box.** `Sort.update` emits `trk.get_state()` =
-  `convert_x_to_bbox(self.kf.x)` ([sort.py:204](../crabs/tracker/sort.py#L204)) — a Kalman-filtered
-  estimate in `(cx, cy, area, aspect_ratio)` space, lagged and with its aspect ratio pulled towards
-  the track's history. The masks are therefore segmented from an approximate box.
-- **Only tracked crabs get a mask.** Detections that SORT suppresses (below `min_hits`) produce no
-  row and no mask. With `min_hits: 1` in the current config this is nearly all of them.
-
-The compute difference is negligible either way: the expensive image encoder runs **once per
-frame** regardless, and prompts only touch the small mask decoder.
-
-
-### 5. Configuration: a separate mask config
-
-The three knobs go in a **new config file of their own**, not into the tracking config:
-
-```yaml
-# crabs/tracker/config/mask_config.yaml   ✨ new file
-sam2_model_id: facebook/sam2.1-hiera-base-plus   # matches the existing script's default
-max_prompts_per_batch: 32                        # see §7
-shard_n_planes: 128                              # see §2; null disables sharding
-```
-
-**Why not a `masks:` block in `tracking_config.yaml`.** Nothing would break if it went there —
-config keys are read individually rather than splatted, so `Sort` is built from three named keys
-([track_video.py:155-159](../crabs/tracker/track_video.py#L155-L159)) and an unknown key is inert.
-The reasons are about shape, not safety:
-
-- [`tracking_config.yaml`](../crabs/tracker/config/tracking_config.yaml) is currently **four flat
-  SORT scalars**. A `masks:` block would be the first nested structure in it, and would put two
-  models' settings in one file for an entry point that only uses one of them.
-- The two have unrelated tuning lifecycles. *Points to discuss* anticipates comparing `-tiny` /
-  `-small` / `-base-plus`, which is mask-config churn that should never touch SORT parameters.
-- It removes a wart rather than documenting one: the integration test fetches its
-  `tracking_config.yaml` from the pooch/GIN registry
-  ([test_inference.py:45](../tests/test_integration/test_inference.py#L45)) and **that file has no
-  `masks:` key**, so a merged block would have to be defensively defaulted. A separate file with
-  its own default path never has the problem.
-- It is the shape issue [#249](https://github.com/SainsburyWellcomeCentre/crabs-exploration/issues/249)
-  ("Uncouple pipeline steps") will want: a later `--tracking_output_dir` mode would pass a mask
-  config and no tracking config at all.
-
-**The new flag is added by inheriting the tracking parser.** `tracking_parse_args` currently builds
-its parser and parses in one function ([track_video.py:350-451](../crabs/tracker/track_video.py#L350-L451)).
-Splitting the building out into `tracking_parser` lets the new entry point reuse every tracking
-argument through argparse's own `parents` mechanism, and adds `--mask_config_file` alongside them:
-
-```diff
---- a/crabs/tracker/track_video.py
-+++ b/crabs/tracker/track_video.py
-+def tracking_parser() -> argparse.ArgumentParser:
-+    """Build a parser with the arguments for tracking.
-+
-+    The parser is defined with ``add_help=False`` so that it can be used as a
-+    parent parser (see ``parents`` in argparse).
-+    """
-+    parser = argparse.ArgumentParser(add_help=False)
-+    ...   # every add_argument call, unchanged
-+    return parser
-+
-+
- def tracking_parse_args(args):
-     """Parse command-line arguments for tracking."""
--    parser = argparse.ArgumentParser()
--    ...
-+    parser = argparse.ArgumentParser(parents=[tracking_parser()])
-     return parser.parse_args(args)
-```
-
-```python
-# crabs/tracker/track_and_mask_video.py
-DEFAULT_MASK_CONFIG = str(Path(__file__).parent / "config" / "mask_config.yaml")
-
-
-def mask_parse_args(args):
-    """Parse command-line arguments for tracking and masking."""
-    parser = argparse.ArgumentParser(parents=[tracking_parser()])
-    parser.add_argument(
-        "--mask_config_file",
-        type=str,
-        default=DEFAULT_MASK_CONFIG,
-        help=(
-            "Location of YAML config to control masking. "
-            "Default: "
-            "crabs-exploration/crabs/tracker/config/mask_config.yaml. "
-        ),
-    )
-    return parser.parse_args(args)
-```
-
-This follows the shape the other entry points already have — one `*_parse_args(args)` per entry
-point, returning one namespace — so `main` and `app_wrapper` take a single `args` as everywhere
-else in the package, and `--mask_config_file` shows up in `detect-and-track-mask --help` next to
-the tracking options.
-
-The `add_help=False` on `tracking_parser` is what makes it usable as a parent: two parsers each
-defining `-h` would raise `ArgumentError` on the child. Both callers therefore wrap it in a real
-parser that adds the help option itself.
-
-`tracking_parse_args` is only called from `app_wrapper` in the same module, so the split has no
-other callers to update.
-
-Read the file as `{**MASK_DEFAULTS, **yaml.safe_load(open(args.mask_config_file))}`. The
-defaults dict is still needed, so that a user's older config missing `shard_n_planes` does not
-fail.
-
-
-### 6. Dependencies
-On dependencies:
-
-- **`sam-2` is not on PyPI, and a plain `pip install` from git pulls a second torch.** The only
-  source is the git URL used in
-  [generate_masks_from_bboxes.py:35](../scripts/generate_masks_from_bboxes.py#L35). SAM2's own
-  [`pyproject.toml`](https://github.com/facebookresearch/sam2/blob/main/pyproject.toml) declares
-  `torch>=2.5.1` as a **build** requirement (its `setup.py` imports `torch.utils.cpp_extension` to
-  build the CUDA extension), so `pip install "sam-2 @ git+..."` downloads an entire extra torch
-  into the isolated build environment — possibly a different variant from the one already
-  installed. That, not the clone, is the slow surprise on the cluster.
-
-  **So declare it as a PEP 735 dependency group, and disable build isolation for it:**
-
-  ```toml
-  # pyproject.toml
-  [dependency-groups]
-  masks = ["sam-2 @ git+https://github.com/facebookresearch/sam2.git"]
-
-  [tool.uv]
-  no-build-isolation-package = ["sam-2"]   # build sam-2 against the env's torch
-  ```
-
-  Installed with `uv sync --group masks` (or `pip install --group masks` on pip ≥ 25.1 — the
-  pips on this machine are older, so I have not run that form).
-
-  Dependency groups are **never written into distribution metadata**, so the sdist built by
-  [test_and_deploy.yml:59](../.github/workflows/test_and_deploy.yml#L59) is untouched — which is
-  what a PEP 508 direct reference in `[project.dependencies]` could not have promised. Everything
-  else about §6 stands: the group is opt-in, the lazy import inside `generate_masks` still raises
-  the actionable error, nothing in the default install pulls SAM2, and CI keeps running the unit
-  suite on ubuntu and macOS without it.
-
-  <details>
-  <summary>Two alternatives, and why not</summary>
-
-  - **An optional extra plus `[tool.uv.sources]`.** Checked rather than assumed: built with
-    uv 0.7.15, both the wheel and the sdist come out carrying `Requires-Dist: sam-2; extra ==
-    "masks"` — the URL *is* stripped, so the sdist would stay publishable. But the published
-    package would then advertise a `sam-2` that PyPI cannot resolve, so `pip install crabs[masks]`
-    fails for anyone not building from this repo. The group has neither problem.
-  - **A prebuilt wheel.** `sam_2-1.0.dist-info/direct_url.json` in the local `.venv` shows the
-    copy currently installed here came from
-    `https://github.com/horsto/sam2/releases/download/v0.0.2/sam_2-1.0-py3-none-any.whl` — pure
-    Python, no build step and no torch build dependency at all. The simplest of the three, at the
-    cost of trusting a third-party fork's release artefact rather than Meta's repo. Worth keeping
-    in mind if the source build proves painful on the cluster.
-  </details>
-
-- **Licence: Apache-2.0 for both the code and the weights.** The `sam-2` package is Apache-2.0
-  (read from the installed `dist-info`), and the checkpoints are too: the Hugging Face model cards
-  for `facebook/sam2.1-hiera-tiny`, `-small`, `-base-plus` and `-large` all declare
-  `license: apache-2.0` in their card metadata (all four checked on 2026-09-11). So there is no
-  licence conflict on either the code or the weights, and none of the `-tiny`/`-small`/`-base-plus`
-  swaps in *Points to discuss* #3 changes that.
-
-- **The conda/pip path still needs a plain command.** [README.md:33-42](../README.md#L33-L42) and
-  the HPC guides install with conda + `pip install -e .[dev]`, and `uv` appears in neither the
-  guides nor [test_and_deploy.yml](../.github/workflows/test_and_deploy.yml). The README section
-  therefore documents both routes: `uv sync --group masks` for a uv checkout, and, for a conda
-  environment with torch already installed,
-
-  ```bash
-  pip install --no-build-isolation "sam-2 @ git+https://github.com/facebookresearch/sam2.git"
-  ```
-
-- **`zarr` is imported directly but not declared** — [`crabs/zarr/create_dataset.py:21`](../crabs/zarr/create_dataset.py#L21)
-  relies on it arriving transitively via `movement`. This proposal adds a second direct importer,
-  so it is worth declaring now, as `zarr>=3` — §2's sharding needs 3.x. (A local `uv.lock` here
-  resolves 3.x, but that file is gitignored ([.gitignore:110](../.gitignore#L110)) and untracked,
-  so it is not evidence about anyone else's environment.)
-
-### 7. Three SAM2 details that will silently degrade or crash this
-
-| Detail | Consequence | Handling |
-|---|---|---|
-| `cv2.VideoCapture.read()` returns **BGR**; `set_image` documents **RGB** (`sam2_image_predictor.py:86-98`) | Silently worse masks — no error, no warning. The existing script never hit this because it reads RGB PNGs via PIL. | `cv2.cvtColor(frame, cv2.COLOR_BGR2RGB)` |
-| `predict()` ends with `masks.squeeze(0)`: returns `(N, 1, H, W)` for N>1 but `(1, H, W)` for N==1 | Crash or wrong axis on any frame with exactly one tracked crab. The same latent bug is in [generate_masks_from_bboxes.py:171](../scripts/generate_masks_from_bboxes.py#L171). | `masks.reshape(len(boxes), *masks.shape[-2:])` |
-| Every prompt gets a **full-frame** mask: `_predict` upsamples to `self._orig_hw`, then `predict()` does `.float().cpu().numpy()` | At 1920×1080 that is 8.29 MB per prompt in float32 — **829 MB on CPU for a 100-crab frame**, and 100 prompts is the exact worst case, since the detector caps detections per image at 100 (§2) | Chunk the prompts (`max_prompts_per_batch`, default 32 → 265 MB peak) and write each chunk's planes to zarr before releasing it |
-
-Also: skip SAM2 entirely on frames with zero tracked boxes, rather than calling `set_image` and
-then `predict` with an empty array.
-
-**Explicitly out of scope:** no ellipse fitting, no orientation, no angle in the CSV, no changes to
-the detector or to SORT. The masks are the deliverable; geometry comes later, offline, from the
-mask store.
-
-
-
----
-
-## Detailed implementation
-
-### The 7 changes
-
-| # | Change | Signature / diff |
-|---|---|---|
-| 1 | **new** `crabs/tracker/track_and_mask_video.py` — the whole feature, ~150 lines | see below |
-| 2 | **new** `crabs/tracker/utils/masks.py` — the read-side helper, ~25 lines | `to_label_image` (§3) |
-| 3 | [`crabs/tracker/track_video.py`](../crabs/tracker/track_video.py) — expose the tracked boxes, and the parser | `+ self.tracked_bboxes_dict = tracked_bboxes_dict` (one line, §1); split `tracking_parse_args` into `tracking_parser()` + `parse_args` (§5) |
-| 4 | **new** `crabs/tracker/config/mask_config.yaml` | the three SAM2/store knobs (§5) |
-| 5 | [`pyproject.toml`](../pyproject.toml) | `detect-and-track-mask = "crabs.tracker.track_and_mask_video:app_wrapper"`; declare `zarr>=3`; add the `masks` dependency group and `[tool.uv] no-build-isolation-package` (§6) |
-| 6 | **new** `tests/test_unit/test_track_and_mask_video.py` | unit tests for the pure helpers (no `sam2` needed) |
-| 7 | [`crabs/tracker/README.md`](../crabs/tracker/README.md) + [`guides/DetectAndTrackHPC.md`](../guides/DetectAndTrackHPC.md) | how to install the `masks` group (§6), pass `--mask_config_file`, and read the store back |
-
-
-<details>
-<summary><b>1. The new module in full outline</b></summary>
-
-Five module-level functions plus a subclass, with the read-side helper in `utils/masks.py`.
-Everything except `load_sam2_predictor` and `predict_masks_for_frame` is pure — no `sam2`, no
-torch, no I/O beyond zarr and the config file — which is what makes the unit tests possible on CI.
-
-```python
-"""Detect, track and mask crabs in a video."""
-
-DEFAULT_MASK_CONFIG = str(Path(__file__).parent / "config" / "mask_config.yaml")
-
-MASK_DEFAULTS = {
-    "sam2_model_id": "facebook/sam2.1-hiera-base-plus",
-    "max_prompts_per_batch": 32,
-    "shard_n_planes": 128,          # §2; None disables sharding
-}
-
-
-def mask_parse_args(args):
-    """Parse command-line arguments for tracking and masking.  [§5]"""
-    parser = argparse.ArgumentParser(parents=[tracking_parser()])   # inherits every tracking arg
-    parser.add_argument("--mask_config_file", type=str, default=DEFAULT_MASK_CONFIG, help=...)
-    return parser.parse_args(args)
-
-
-def load_mask_config(path) -> dict:
-    """Read the mask config, backfilled with MASK_DEFAULTS.  [§5]"""
-    with open(path) as f:
-        return {**MASK_DEFAULTS, **(yaml.safe_load(f) or {})}
-
-
-def create_mask_zarr(path, n_frames, n_track_ids, image_shape, metadata_dict) -> zarr.Array:
-    """Open a (T, M, H, W) bool array named "masks" in a group at `path`.
-
-    chunks=(1, 1, H, W), shards=(1, shard_n_planes, H, W), fill_value=False.
-    Asserts the array dtype is bool, matching metadata["mask_encoding"]
-    == "instance_planes" — the guard that keeps §2's warning from recurring.
-    """
-
-
-def load_sam2_predictor(model_id: str, device: str):
-    """Import sam2 lazily and return a SAM2ImagePredictor.  [§6]"""
-    try:
-        from sam2.sam2_image_predictor import SAM2ImagePredictor
-    except ImportError as e:
-        raise ImportError(
-            "detect-and-track-mask needs SAM2. Install it with:\n"
-            "  uv sync --group masks\n"
-            "or, in a conda env with torch already installed:\n"
-            '  pip install --no-build-isolation "sam-2 @ git+https://github.com/facebookresearch/sam2.git"'
-        ) from e
-    return SAM2ImagePredictor.from_pretrained(model_id, device=device)
-
-
-def predict_masks_for_frame(predictor, frame_bgr, boxes, max_prompts_per_batch):
-    """(H, W, 3) BGR + (N, 4) boxes -> (N, H, W) bool.  [§5]"""
-    predictor.set_image(cv2.cvtColor(frame_bgr, cv2.COLOR_BGR2RGB))
-    out = []
-    FOR EACH chunk OF boxes, size max_prompts_per_batch:
-        masks, _iou, _low_res = predictor.predict(box=chunk, multimask_output=False)
-        out.append(masks.reshape(len(chunk), *masks.shape[-2:]).astype(bool))
-    RETURN np.concatenate(out)
-
-
-class TrackingAndMasking(Tracking):
-    """Extend Tracking with a SAM2 masking pass over the tracked boxes."""
-
-    def __init__(self, args, mask_config):
-        super().__init__(args)           # unchanged; reads the tracking config
-        self.mask_config = mask_config   # separate file, §5
-
-    def prep_mask_outputs(self): ...     # output path + M (below)
-    def generate_masks(self): ...        # the loop, below
-
-
-def main(args):
-    inference = TrackingAndMasking(args, load_mask_config(args.mask_config_file))
-    inference.detect_and_track_video()   # inherited, unchanged
-    inference.generate_masks()
-
-
-def app_wrapper():
-    logging.getLogger().setLevel(logging.INFO)
-    torch.set_float32_matmul_precision("medium")
-    main(mask_parse_args(sys.argv[1:]))   # same shape as the other entry points, §5
-```
-
-`generate_masks`, mirroring the structure of `write_all_video_frames_as_images`
-([io.py:205](../crabs/tracker/utils/io.py#L205)):
-
-```python
-def generate_masks(self):
-    predictor = load_sam2_predictor(cfg["sam2_model_id"], self.accelerator)
-
-    # M is knowable here: tracking has already finished  [§2]
-    n_track_ids = max(
-        frame["ids"].max() for frame in self.tracked_bboxes_dict.values() if len(frame["ids"])
-    )
-    mask_zarr = create_mask_zarr(path, total_n_frames, n_track_ids, (H, W), metadata)
-
-    input_video_object = open_video(self.input_video_path)
-    frame_idx = 0
-    WHILE input_video_object.isOpened():
-        ret, frame = input_video_object.read()
-        IF not ret:
-            parse_video_frame_reading_error_and_log(frame_idx, total_n_frames)
-            break
-
-        frame_data = self.tracked_bboxes_dict[frame_idx]
-        boxes, ids = frame_data["tracked_boxes"], frame_data["ids"]
-        IF len(boxes) > 0:                                    # [§5]
-            masks = predict_masks_for_frame(predictor, frame, boxes, chunk_size)
-            mask_zarr.oindex[frame_idx, ids.astype(int) - 1] = masks   # [§3]
-        frame_idx += 1
-
-    input_video_object.release()
-```
-
-Frames with no tracked boxes are left at the store's `fill_value=False`, and those chunks are never
-written — correct by construction, and one fewer branch.
-
-Two invariants worth asserting at the end of the run, both cheap and both about the ID↔axis
-mapping the format now depends on: every emitted track ID is in `1..M`, and
-`KalmanBoxTracker.count >= M` (it can exceed `M` when a tracker was suppressed below `min_hits`,
-never fall below it).
-
-**Rejected alternative — masking inside `core_detection_and_tracking`.** It saves one video pass,
-but it puts SAM2 in the middle of the detection loop, changes an existing method, and makes the
-feature impossible to skip. A fourth pass matches the pattern the file already uses twice and
-keeps the diff to `track_video.py` down to the one added line.
-
-**Rejected alternative — a post-pass over an existing `_tracks.csv`.** More decoupled, and
-attractive later, but it means re-parsing VIA JSON and re-deriving the frame index from filenames
-when the same data is already in memory. It also could not reuse `Tracking` at all. Worth
-revisiting once the format is settled.
-</details>
-
-<details>
-<summary><b>2. Formats considered and rejected</b></summary>
-
-All four are lossless-or-not on overlap, and all four are `regionprops`-shaped at read time. The
-choice came down to what the downstream access pattern is.
-
-| Format | Lossless on overlap | `masks[:, tid]` per-crab slice | Opens in napari as-is | Note |
-|---|---|---|---|---|
-| **`(T, M, H, W)` bool, plane per track ID** ✅ chosen | yes | **yes, one slice** | no | chunk count is the cost (§2) |
-| `(T, H, W)` int32 label image | **no** | no | yes | simplest; discards the lower-ID crab's overlapping pixels |
-| `(T, K, H, W)` int32 overlap layers | yes | no | plane 0 ≈ whole frame | `K`≈2–4; needs a greedy packing pass at write time |
-| bbox-local crops + index table | yes | no (needs a scan) | no | ~500× smaller raw, but ragged and no time-slicing |
-
-**Why not the label image.** It was the original proposal. It cannot represent two crabs on one
-pixel, and the ellipse fits Phase 3 wants would be biased by exactly the neighbours that touch
-most. Rejected on losslessness, not on size — zarr compresses a label image well enough that size
-was never the deciding argument.
-
-**Why not overlap layers.** Genuinely close, and better on chunk count and napari. It loses because
-the axis carries no identity, so `masks[:, tid]` — one crab's whole trajectory in a single read —
-is not expressible. That slice is what the orientation work will be built on.
-
-**Why not crops.** Much the smallest, and the only one that scales to long clips without thought.
-Rejected for now because it is ragged (two arrays plus offset bookkeeping), needs a scan to
-assemble one crab's time series, and cannot be read without the repo's helpers. Worth revisiting
-if *Points to discuss* #1 or #2 comes back badly.
-</details>
-
----
-
-## Files changed
-
-| File | Change |
-|---|---|
-| `crabs/tracker/track_and_mask_video.py` | **new** — the entire write path |
-| `crabs/tracker/utils/masks.py` | **new** — `to_label_image` (§3) |
-| [`crabs/tracker/track_video.py`](../crabs/tracker/track_video.py) | one added line (store the tracked boxes on `self`); split `tracking_parse_args` into `tracking_parser()` + `parse_args`, so the new entry point can inherit it with `parents=[...]` (§5) |
-| `crabs/tracker/config/mask_config.yaml` | **new** — SAM2 and store knobs, separate from the tracking config (§5) |
-| [`pyproject.toml`](../pyproject.toml) | new `detect-and-track-mask` script; declare `zarr>=3`; `[dependency-groups] masks` + `[tool.uv] no-build-isolation-package` (§6) |
-| `tests/test_unit/test_track_and_mask_video.py` | **new** |
-| [`notebooks/notebook_visualise_masks_from_zarr.py`](../notebooks/notebook_visualise_masks_from_zarr.py) | drop the one-hot `map_blocks` expansion ([:142-174](../notebooks/notebook_visualise_masks_from_zarr.py#L142-L174)) — the store is already 4-D; route the two napari calls through `to_label_image` (§3) |
-| [`crabs/tracker/README.md`](../crabs/tracker/README.md) | document the entry point, `--mask_config_file`, the store layout and how to read it back |
-| [`guides/DetectAndTrackHPC.md`](../guides/DetectAndTrackHPC.md) | installing the `masks` group on the cluster (and the `--no-build-isolation` pip line for the conda envs, §6); staging the second config file |
-
-Nothing else is touched. In particular: no change to `sort.py`, to `utils/io.py`, to
-`tracking_config.yaml`, to the CSV format, to `evaluate_tracker.py`, or to the detector.
-
----
-
-## Overview of tests to write
-
-**Pure unit — must pass with no `sam2` installed (this is the CI shape)**
-
-1. **The ID↔axis mapping, which is now the core contract.** Write three masks with IDs `[7, 3, 12]`
-   via `oindex[t, ids - 1]`, then assert `masks[t, 6]`, `masks[t, 2]` and `masks[t, 11]` are the
-   masks that went in, and that every other plane in frame `t` is all-`False`.
-2. **Overlap is preserved.** Two masks with IDs `3` and `12` sharing a block of pixels: both planes
-   still contain the shared pixels in full, and each plane's `sum()` equals its input's. This is
-   the test that would fail under the label-image format, and it is the reason for the change.
-3. `to_label_image`: non-overlapping planes give a label image whose non-zero values are exactly
-   `{3, 7, 12}`, regions in the right places; overlapping planes resolve by `policy`, and the
-   result does not depend on plane order.
-4. Round-trip through the consumer: feed `to_label_image` output to `skimage.measure.regionprops`
-   and assert `{p.label for p in props} == {3, 7, 12}`. The property the read path exists for.
-5. A frame with no tracked boxes leaves every plane all-`False` and does not raise.
-6. `create_mask_zarr`: dtype is `bool`, `fill_value` is `False`, chunks are `(1, 1, H, W)`, shards
-   are `(1, 128, H, W)`, and `.attrs["mask_encoding"] == "instance_planes"` — plus a test that the
-   internal assertion **fires** if the dtype and the declared encoding disagree (the guard that
-   keeps §2's `dtype="bool"` bug from being reintroduced from the other direction).
-7. `load_sam2_predictor` raises `ImportError` with the install command in the message when `sam2`
-   is absent (`monkeypatch` the import) — assert the message names `uv sync --group masks`, so it
-   cannot drift from the group actually declared in `pyproject.toml`.
-8. **`mask_parse_args` (§5).** Three cases, since this is the only new CLI surface: the namespace
-   carries `mask_config_file` when the flag is passed, and `DEFAULT_MASK_CONFIG` when it is not;
-   every tracking argument is inherited (assert that `vars(mask_parse_args(argv + mask_flag))`
-   minus `mask_config_file` equals `vars(tracking_parse_args(argv))` for the same `argv`); and
-   `--help` exits `0` with both `--mask_config_file` and `--config_file` in the usage text — the
-   property the `parents=[...]` refactor buys, and the one that would regress if
-   `tracking_parser` were ever given `add_help=True`.
-9. `load_mask_config`: a config missing `shard_n_planes` is backfilled from `MASK_DEFAULTS`, and
-   an empty file yields the defaults rather than raising — the guard against
-   `yaml.safe_load` returning `None`.
-
-**Existing tests that must stay green, unmodified**
-
-10. `pytest tests/test_unit` — in particular
-   [`test_tracking_io.py`](../tests/test_unit/test_tracking_io.py) (the CSV is untouched) and
-   [`test_track_video.py`](../tests/test_unit/test_track_video.py) (neither the one-line change to
-   `detect_and_track_video` nor the parser split must alter behaviour). The parser split has no
-   existing test covering it — `tracking_parse_args` is called only from `app_wrapper` — so add
-   one: `tracking_parse_args` on a minimal valid argv still returns the same defaults, and
-   `detect-and-track-video --help` still lists every tracking option.
-11. [`test_entry_points.py`](../tests/test_unit/test_entry_points.py) — extend with
-   `detect-and-track-mask` following the existing pattern.
-
-**Integration (slow, opt-in)**
-
-12. A `test_detect_and_track_mask` alongside
-   [`test_detect_and_track_video`](../tests/test_integration/test_inference.py), reusing the
-   `pooch_registry` fixture and the 3-frame clip, `@pytest.mark.skipif` on `sam2` being importable.
-   Assert the store exists, has shape `(3, M, H, W)` and dtype `bool`, and that for each frame the
-   set of planes with any `True` pixel is exactly `{tid - 1 for tid in that frame's CSV rows}`.
-   That last assertion is the real contract of this feature.
-
-   Note this test now passes the registry's `tracking_config.yaml` **unchanged** and lets
-   `--mask_config_file` fall back to its packaged default — with a separate config there is nothing
-   to add to the GIN registry file (§5).
-
----
-
-## Verifications for agent to run
-
-```bash
-# lint + full unit suite (no sam2 needed)
-pre-commit run --all-files
-pytest tests
-
-# slow end-to-end CLI tests (pooch downloads test data on first run)
-pytest -m slow tests/test_integration/test_inference.py
-```
-
-Manual check on a real clip:
-
-```bash
-uv sync --group masks   # §6; in a conda env instead:
-# pip install --no-build-isolation "sam-2 @ git+https://github.com/facebookresearch/sam2.git"
-
-detect-and-track-mask \
-    --trained_model_path <ckpt> \
-    --video_path <clip.mp4> \
-    --mask_config_file <mask_config.yaml> \   # optional; defaults to the packaged one
-    --config_file <tracking_config.yaml> \
-    --accelerator=gpu \
-    --output_dir_no_timestamp
-```
-
-then confirm the contract and eyeball the masks:
-
-```python
-import zarr, numpy as np
-from skimage.measure import regionprops
-from crabs.tracker.utils.masks import to_label_image
-
-root = zarr.open("tracking_output/<clip>_masks.zarr", mode="r")
-masks = root["masks"]
-print(masks.shape, masks.dtype, dict(root.attrs))   # (T, M, H, W) bool
-
-# the contract: which planes are populated in frame 0
-print(sorted(np.flatnonzero(masks[0].any(axis=(1, 2))) + 1))   # == track IDs in frame 0 of the CSV
-
-# the read path
-props = regionprops(to_label_image(masks[0]))
-print([p.area for p in props][:10])       # sanity: no zero-area or whole-frame regions
-```
-
-[`notebooks/notebook_visualise_masks_from_zarr.py`](../notebooks/notebook_visualise_masks_from_zarr.py)
-loads this shape into napari once its one-hot expansion is dropped (*Files changed*); overlaying it
-on the frames written by `--save_frames` is the fastest qualitative check that the masks are on the
-crabs and not on the background.
-
-Also worth recording once, since both decide the format's viability: `du -sh` **and the file count**
-(`find ... | wc -l`) of the store for one real clip, and the wall-clock time of the masking pass
-versus the detection pass.
-
----
-
-## Points to discuss
-
-1. **Chunk count is this format's one weak spot, and it is unmeasured.** Per-instance planes mean
-   ~`n_frames × n_crabs` ≈ 300,000 chunks for a 3000-frame clip (§2). Sharding at
-   `(1, 128, H, W)` should cut that to `n_frames × ceil(M/128)`, but I have not measured either the
-   file count or whether zarr 3's sharding writer behaves well with `oindex` writes of a frame's
-   scattered IDs — it may do a read-modify-write per shard. This is the number I would check first
-   on the cluster, and `shard_n_planes` is a config value precisely so it can be tuned without a
-   format change. If sharding disappoints, the fallback is a zip store.
-2. **Store size.** Raw `(T, M, H, W)` bool is enormous — 622 GB for `T=3000, M=100` — but it is
-   never stored: each plane is ~0.17% non-zero, all-`False` chunks are not written at all, and only
-   ~`n_frames × n_crabs` chunks ever exist. My estimate is the same order as the label image would
-   have compressed to (a few hundred MB to ~1 GB), but I have not measured it and will not guess.
-   Note this format has no `uint16`/`int32` lever to pull because there is no ID in the data — if
-   it comes out too big the levers are a stronger compressor, or storing bbox-local crops rather
-   than full-frame planes (which would drop the raw volume by ~500× but costs the direct
-   `masks[:, tid-1]` slicing that motivated this shape).
-3. **`sam2_model_id` default.** I have matched the existing script's `-base-plus`
-   ([generate_masks_from_bboxes.py:307](../scripts/generate_masks_from_bboxes.py#L307)) so the two
-   agree. `-tiny` / `-small` are considerably faster and may well be enough at this object size —
-   easy to compare once the entry point exists, since it is a config value, and with the separate
-   mask config (§5) swapping it never touches the tracking parameters.
-
-5. **Should the entry point re-run detection, or accept an existing tracking output?**
-    * As proposed
-   it re-runs everything, which is the simplest thing and matches the name.
-   * The cost is that
-   iterating on SAM2 settings means re-running the detector each time.
-   * A `--tracking_output_dir`
-   flag that skips straight to `generate_masks` would fix that and fits issue
-   [#249](https://github.com/SainsburyWellcomeCentre/crabs-exploration/issues/249)
-   ("Uncouple pipeline steps"), but it needs the tracks CSV parsed back in. Deliberately left out;
-   easy to add later without changing the format — and the separate mask config (§5) is already the
-   right shape for it, since that mode would pass a mask config and no tracking config at all.
-
-6. **⚠️ The detector is running at its detection cap.**
-    * `fasterrcnn_resnet50_fpn_v2` is constructed
-   with no kwargs ([models.py:82](../crabs/detector/models.py#L82)), so torchvision's default
-   `box_detections_per_img=100` applies — and this scene has **~100 crabs per frame**.
-   * Dense frames
-   are therefore plausibly being truncated to the top 100 by score, silently, before tracking and
-   before any masking.
-   * This does not change anything in this proposal — the masks follow whatever
-   the tracker emits — but it caps what the masks can ever cover, and it is invisible in the
-   outputs.
-   * Worth its own issue and a quick check: log
-   `max(len(boxes) for boxes in detections)` over a real clip and see how often it sits at exactly 100.
-
-7. **The dependency group only helps a uv checkout, and this repo is not one yet.**
-    * `uv.lock` is
-   gitignored and untracked, and the install docs are conda + pip throughout, so `uv sync --group
-   masks` documents a workflow the repo does not otherwise have — which is why §6 keeps the
-   `pip install --no-build-isolation` line alongside it.
-   * If you would rather commit `uv.lock` and
-   make uv the documented path (which would also pin the SAM2 commit, rather than tracking
-   whatever `main` is on the day someone installs), that is a bigger, separate decision and I have
-   not assumed it here.
-8. **Two more things found while reading, both out of scope.**
-    (a) `--max_frames_to_read` is parsed at
-   [track_video.py:440](../crabs/tracker/track_video.py#L440) and never used by anything — see PR
-   [#245](https://github.com/SainsburyWellcomeCentre/crabs-exploration/pull/245). It would make
-   iterating on this entry point much cheaper (because we could run things for a limited amount of frames).
-
-   (b) `write_tracked_detections_to_csv` zips the
-   tracked boxes against `detections_dict["scores"]`, which holds *all raw, unthresholded*
-   detections in detector order ([track_video.py:267](../crabs/tracker/track_video.py#L267),
-   [io.py:77-82](../crabs/tracker/utils/io.py#L77-L82)) — different length and different order, so
-   the CSV `confidence` column is effectively arbitrary.
-
-   Neither affects the masks; both are worth
-   separate PRs.

From cd9e0ceff712a779068549f54fb706246cc9c954 Mon Sep 17 00:00:00 2001
From: sfmig <33267254+sfmig@users.noreply.github.com>
Date: Wed, 16 Sep 2026 13:54:53 +0100
Subject: [PATCH 07/19] Anchor the tracks zarr `time` coordinate to the clip

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>
---
 crabs/zarr/create_dataset.py | 70 ++++++++++++++++++++++++++++++++++--
 1 file changed, 67 insertions(+), 3 deletions(-)

diff --git a/crabs/zarr/create_dataset.py b/crabs/zarr/create_dataset.py
index fa02c464..1c828fbe 100644
--- a/crabs/zarr/create_dataset.py
+++ b/crabs/zarr/create_dataset.py
@@ -66,8 +66,16 @@ def load_extended_ds(
         added metadata as coordinates and data variables.
 
     """
-    # Load VIA tracks file as movement dataset
-    ds = load_bboxes.from_via_tracks_file(via_tracks_file_path)
+    # Load VIA tracks file as movement dataset.
+    # We request the frame numbers as defined in the file. With the
+    # default (use_frame_numbers_from_file=False), `movement` maps them to
+    # a 0-based sequence of *consecutive* integers. Since the tracker
+    # writes no rows for frames with no tracked boxes, that would shift
+    # every subsequent frame earlier, and `time` would no longer be the
+    # clip's frame index.
+    ds = load_bboxes.from_via_tracks_file(
+        via_tracks_file_path, use_frame_numbers_from_file=True
+    )
 
     # Extract metadata for this row
     clip_filename = _via_tracks_to_clip_filename(ds.attrs["source_file"])
@@ -77,14 +85,43 @@ def load_extended_ds(
     global_clip_start_frame_0idx = row["loop_START_frame_ffmpeg"] - 1
     global_clip_end_frame_0idx = row["loop_END_frame_ffmpeg"] - 1
     global_escape_start_frame_0idx = row["escape_START_frame_0_based_idx"]
+    n_clip_frames = (
+        global_clip_end_frame_0idx - global_clip_start_frame_0idx + 1
+    )
+
+    # Check the frames in the VIA tracks file belong to this clip
+    _validate_frames_in_clip_range(
+        ds.time.values,
+        n_clip_frames,
+        global_clip_start_frame_0idx,
+        clip_filename,
+    )
+
+    # Pad the time coordinate to span the full clip.
+    # `movement` pads with NaNs the individuals that are missing from a
+    # frame, but a frame that no individual appears in is absent from the
+    # time coordinate altogether, so we reindex to add it back.
+    n_frames_wout_boxes = n_clip_frames - ds.sizes["time"]
+    if n_frames_wout_boxes:
+        print(
+            f"{clip_filename}: {n_frames_wout_boxes} of {n_clip_frames} "
+            "frames have no tracked boxes; padding them with NaNs."
+        )
+    ds = ds.reindex(time=np.arange(n_clip_frames))
 
     # Add escape_state as data variable
     local_escape_start_frame_0idx = (
         global_escape_start_frame_0idx - global_clip_start_frame_0idx
     )
+    if not 0 <= local_escape_start_frame_0idx < n_clip_frames:
+        raise ValueError(
+            f"{clip_filename}: the escape start frame "
+            f"({local_escape_start_frame_0idx} as a clip frame index) is "
+            f"outside the clip's frame range 0-{n_clip_frames - 1}."
+        )
     # we use float16 (not int/bool) to allow for NaN padding after
     # concatenating along clip_id
-    escape_state = np.zeros(ds.time.shape[0], dtype=np.float16)
+    escape_state = np.zeros(n_clip_frames, dtype=np.float16)
     escape_state[local_escape_start_frame_0idx:] = 1.0
     ds["escape_state"] = ("time", escape_state)
 
@@ -118,6 +155,33 @@ def load_extended_ds(
     return ds
 
 
+def _validate_frames_in_clip_range(
+    frames_0idx: np.ndarray,
+    n_clip_frames: int,
+    clip_first_frame_0idx: int,
+    clip_filename: str,
+) -> None:
+    """Check the frame numbers in a VIA tracks file belong to the clip.
+
+    The frame numbers are expected to be 0-based indices into the clip,
+    which is how `detect-and-track-video` numbers the frames of the clip
+    it is given.
+    """
+    if frames_0idx.min() >= 0 and frames_0idx.max() < n_clip_frames:
+        return
+
+    raise ValueError(
+        f"{clip_filename}: the VIA tracks file holds frame numbers "
+        f"{frames_0idx.min()}-{frames_0idx.max()}, outside the clip's "
+        f"frame range 0-{n_clip_frames - 1}. Frame numbers are expected "
+        f"to be 0-based indices into the clip, which starts at video "
+        f"frame {clip_first_frame_0idx}. Either the file was tracked on "
+        f"the full video or paired with the wrong metadata row, or the "
+        f"cut clip does not have the frame count the metadata csv states "
+        f"(re-run extract-loop-clips --verify_frames)."
+    )
+
+
 def _group_files_per_video(
     files_dir: str | Path,
     glob_pattern: str,

From f94b883e3b09cbea5dcdf9645b7a3b8311a92aaa Mon Sep 17 00:00:00 2001
From: sfmig <33267254+sfmig@users.noreply.github.com>
Date: Wed, 16 Sep 2026 13:57:50 +0100
Subject: [PATCH 08/19] Assert the values of `time` and `escape_state`, not
 just their presence

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>
---
 tests/test_unit/test_create_zarr_dataset.py | 102 +++++++++++++++++++-
 1 file changed, 99 insertions(+), 3 deletions(-)

diff --git a/tests/test_unit/test_create_zarr_dataset.py b/tests/test_unit/test_create_zarr_dataset.py
index 8b3a75d3..643432b7 100644
--- a/tests/test_unit/test_create_zarr_dataset.py
+++ b/tests/test_unit/test_create_zarr_dataset.py
@@ -2,6 +2,7 @@
 from contextlib import nullcontext as does_not_raise
 from pathlib import Path
 
+import numpy as np
 import pandas as pd
 import pytest
 import xarray as xr
@@ -24,11 +25,16 @@
 def sample_via_tracks_file_factory(tmp_path):
     """Return factory of VIA tracks files with custom filenames."""
 
-    def _sample_via_tracks_file(filename):
-        """Create a mock VIA tracks file."""
+    def _sample_via_tracks_file(filename, frame_indices=None):
+        """Create a mock VIA tracks file.
+
+        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.
+        """
         # Add a few frames with bounding box annotations
         via_data = []
-        for frame_idx in [0, 10, 20, 50, 60, 80, 100]:
+        for frame_idx in frame_indices or [0, 10, 20, 50, 60, 80, 100]:
             via_data.append(
                 {
                     "filename": f"frame_{frame_idx:08d}.png",
@@ -151,6 +157,96 @@ def test_load_extended_ds(
     assert "clip_escape_first_frame_0idx" in ds.coords
     assert "clip_escape_type" in ds.coords
 
+    # Check time spans the full clip, as the clip's 0-based frame index
+    assert ds.sizes["time"] == 101
+    assert np.array_equal(ds.time.values, np.arange(101))
+
+    # Check the escape starts at clip frame 51
+    # (global frame 150, clip starting at global frame 99)
+    escape_state = ds.escape_state.values.ravel()
+    assert int(np.argmax(escape_state)) == 51
+    assert escape_state.sum() == 101 - 51
+
+    # Check frames with no boxes are all-NaN rows, rather than missing
+    # (the VIA tracks file holds boxes for frames 0, 10, 20, 50, 60, 80, 100)
+    assert ds.position.isel(time=1).isnull().all()
+    assert not ds.position.isel(time=10).isnull().any()
+
+
+def test_load_extended_ds_contiguous_file(
+    sample_via_tracks_file_factory, sample_metadata_df_factory
+):
+    """Test a VIA tracks file with one row per frame of the clip.
+
+    It should give the same `time` and `escape_state` as the sparse file,
+    and no NaN rows.
+    """
+    via_tracks_path = sample_via_tracks_file_factory(
+        "04.09.2023-01-Right-Loop05_tracks.csv",
+        frame_indices=list(range(101)),
+    )
+    df_metadata = sample_metadata_df_factory([via_tracks_path])
+
+    ds = load_extended_ds(via_tracks_path, df_metadata)
+
+    assert np.array_equal(ds.time.values, np.arange(101))
+    escape_state = ds.escape_state.values.ravel()
+    assert int(np.argmax(escape_state)) == 51
+    assert escape_state.sum() == 101 - 51
+    assert not ds.position.isnull().any()
+
+
+@pytest.mark.parametrize(
+    "frame_indices",
+    [
+        pytest.param(
+            list(range(99, 200)),
+            id="tracked_on_full_video_or_wrong_metadata_row",
+        ),
+        pytest.param(
+            list(range(102)), id="cut_clip_longer_than_metadata_states"
+        ),
+    ],
+)
+def test_load_extended_ds_frames_out_of_clip_range(
+    frame_indices, sample_via_tracks_file_factory, sample_metadata_df_factory
+):
+    """Test the guard on frame numbers outside the clip's frame range.
+
+    Without it, `reindex` would silently drop the out-of-range rows.
+    """
+    via_tracks_path = sample_via_tracks_file_factory(
+        "04.09.2023-01-Right-Loop05_tracks.csv", frame_indices=frame_indices
+    )
+    df_metadata = sample_metadata_df_factory([via_tracks_path])
+
+    with pytest.raises(ValueError) as exc_info:
+        load_extended_ds(via_tracks_path, df_metadata)
+
+    # The message reports the file's own range and the clip's range
+    message = str(exc_info.value)
+    assert f"{frame_indices[0]}-{frame_indices[-1]}" in message
+    assert "0-100" in message
+
+
+def test_load_extended_ds_escape_frame_out_of_clip_range(
+    sample_via_tracks_file_factory, sample_metadata_df_factory
+):
+    """Test the guard on an escape frame outside the clip's frame range."""
+    via_tracks_path = sample_via_tracks_file_factory(
+        "04.09.2023-01-Right-Loop05_tracks.csv"
+    )
+    df_metadata = sample_metadata_df_factory([via_tracks_path])
+
+    # Clip spans global frames 99-199; put the escape past its end
+    df_metadata.loc[0, "escape_START_frame_0_based_idx"] = 250
+
+    with pytest.raises(ValueError) as exc_info:
+        load_extended_ds(via_tracks_path, df_metadata)
+
+    assert "escape start frame" in str(exc_info.value)
+    assert "0-100" in str(exc_info.value)
+
 
 @pytest.mark.parametrize(
     "list_files, expected_exception",

From 84030ddf6a08a496d90c5d255a8e038d861475d7 Mon Sep 17 00:00:00 2001
From: sfmig <33267254+sfmig@users.noreply.github.com>
Date: Wed, 16 Sep 2026 13:58:04 +0100
Subject: [PATCH 09/19] Document what `time` means in the tracks zarr store

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>
---
 guides/CreateZarrDatasetForTracks.md | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/guides/CreateZarrDatasetForTracks.md b/guides/CreateZarrDatasetForTracks.md
index 74e09a4e..fafb5d19 100644
--- a/guides/CreateZarrDatasetForTracks.md
+++ b/guides/CreateZarrDatasetForTracks.md
@@ -80,6 +80,21 @@
 
     If the array job runs successfully, a zarr dataset named `CrabTracks-slurm<SLURM_ARRAY_JOB_ID>.zarr` will be generated in the location specified by `ZARR_STORE_OUTPUT`. Each group in the zarr dataset will correspond to a `movement` [bounding box dataset](https://movement.neuroinformatics.dev/latest/user_guide/movement_dataset.html) containing all tracks for one video.
 
+    The `time` coordinate is the clip's 0-based frame index, and it always spans the whole clip: frames that hold no tracked crabs are stored as NaNs rather than dropped. So within a clip, row `i` is frame `i`.
+
+    To get a single clip's full frame range, without the NaN padding that the concatenation along `clip_id` adds beyond the end of shorter clips:
+
+    ```python
+    d = ds_video.isel(clip_id=0)
+    n_clip_frames = int(d.clip_last_frame_0idx - d.clip_first_frame_0idx) + 1
+    d_clip = d.isel(time=slice(0, n_clip_frames))
+    ```
+
+    Prefer this to `dropna(dim="time", how="all")`, which cannot tell "beyond this clip's end" from "no crabs in this frame": it silently drops crab-free frames from the middle of the clip and returns a gapped time axis. The slice keeps every frame of the clip and nothing else.
+
+    > [!WARNING]
+    > Zarr stores built before the `time` coordinate was anchored to the clip have a `time` coordinate that does not mean this. There, `time` was a 0-based count of the frames *with detections*, so frames with no tracked crabs shifted every later frame earlier, and `escape_state` switched on at the wrong frame. Those stores may need rebuilding.
+
     To inspect this dataset, please refer to the example notebooks in the 🦀 repository.
 
 ### Re-running failed jobs

From 1bc3070f1c13093fad1a9e7011642ee94a3c63c6 Mon Sep 17 00:00:00 2001
From: sfmig <33267254+sfmig@users.noreply.github.com>
Date: Wed, 16 Sep 2026 15:53:02 +0100
Subject: [PATCH 10/19] Remove plan

---
 claude-plans/proposal-zarr-time-coordinate.md | 558 ------------------
 1 file changed, 558 deletions(-)
 delete mode 100644 claude-plans/proposal-zarr-time-coordinate.md

diff --git a/claude-plans/proposal-zarr-time-coordinate.md b/claude-plans/proposal-zarr-time-coordinate.md
deleted file mode 100644
index 9353d90a..00000000
--- a/claude-plans/proposal-zarr-time-coordinate.md
+++ /dev/null
@@ -1,558 +0,0 @@
-# Proposal for anchoring the `time` coordinate of the tracks zarr dataset to the clip
-
-## Description
-
-The `time` coordinate of the tracks zarr dataset is not the clip's frame index, so `escape_state`
-marks the escape on the wrong frame. This proposal makes the `time` axis dense, in
-[`load_extended_ds`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L49).
-
-Two things change, and they are worth keeping apart.
-* `time` becomes the clip's **frame index**, and
-* `time` becomes **dense**.
-
-As a result, the axis is uniformly sampled and a frame
-with no detections is a NaN row rather than an absent time coordinate.
-
-Only the first is needed to fix the bug in `escape_state` (although it could also be fixed differently). The second is the larger claim, and it is argued in [§5](#5-why-time-should-be-dense) — it is the
-reason the *Rejected alternative* under [Detailed implementation](#detailed-implementation) is
-rejected.
-
-```mermaid
-flowchart TD
-    A["&lt;clip&gt;_tracks.csv<br/><i>rows only for frames with tracked crabs</i>"] --> B
-    B["load_bboxes.from_via_tracks_file<br/>✨ use_frame_numbers_from_file=True"] --> C
-    C["ds.time = real frame numbers<br/><i>frames with no crabs are still missing</i>"] --> D
-    D["✨ ds.reindex(time=arange(n_clip_frames))<br/><i>n_clip_frames = loop_END_frame_ffmpeg<br/>- loop_START_frame_ffmpeg + 1</i>"] --> E
-    E["ds.time = 0 … n_clip_frames-1<br/><b>row i is clip frame i</b>"] --> F
-    F["escape_state, over the full clip"] --> G
-    G["temp store: one group per clip"] --> H
-    H["xr.concat(dim='clip_id', join='outer')"] --> I
-    I["final store: one group per video"]
-    style B fill:#e0e7ff,stroke:#4338ca,color:#1e1b4b
-    style D fill:#e0e7ff,stroke:#4338ca,color:#1e1b4b
-    style E fill:#e0e7ff,stroke:#4338ca,color:#1e1b4b
-```
-
-Arrows are the data path of one clip, from its VIA tracks file to the final zarr store. ✨ marks
-what this pull request adds.
-
-Node → code:
-
-- `B`, `C`, `D`, `E`, `F` — [`load_extended_ds`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L49)
-- `G` — [`create_temp_zarr_store`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L203)
-- `H`, `I` — [`create_final_zarr_store`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L277)
-
-## References
-
-
-- [`guides/CreateZarrDatasetForTracks.md`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/guides/CreateZarrDatasetForTracks.md)
-  — how the dataset is built on the cluster.
-- [`movement.io.load_bboxes.from_via_tracks_file`](https://github.com/neuroinformatics-unit/movement/blob/main/movement/io/load_bboxes.py)
-  — the loader whose default behaviour partially causes the issue (to be changed in [PR1103](https://github.com/neuroinformatics-unit/movement/pull/1103)). Behaviour checked against movement 0.16.0, the 0.17.0
-  source, and `main`; identical in all three.
-- No open pull request touches `crabs/zarr/`.
-
-## Overview of steps
-
-1. Load each VIA tracks file with `use_frame_numbers_from_file=True`.
-2. Derive the clip's frame count from the metadata csv.
-3. Raise if the frame numbers in the file fall outside the clip's frame range.
-4. Reindex the dataset onto the clip's full frame range.
-5. Build `escape_state` over that frame range, and raise if the escape frame falls outside it.
-6. Strengthen `test_load_extended_ds` to assert the values of `escape_state`, not just its presence.
-
-## Key aspects of suggested implementation
-
-### 1. `movement` pads individuals, not frames
-
-`from_via_tracks_file` sizes the time axis as `n_frames = len(np.unique(frame_numbers))`
-* these are the frames **with data**, not `max - min + 1`. The dense grid it returns is *(unique frames
-present) × (unique individuals present)*:
-- a frame that **no** individual appears in is absent from the time
-  coordinate altogether.
-
-[`write_tracked_detections_to_csv`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/tracker/utils/io.py#L75-L101)
-iterates over the boxes *within* a frame, so a frame with no tracked crabs contributes no rows at
-all. Those frames disappear.
-
-On a 6-frame clip whose VIA tracks file has rows only for frames 0, 1, 3 and 5, with crab 2 also
-missing from frame 1:
-
-```python
-# frame numbers as written in the file:
-use_frame_numbers_from_file=True
-  ds.time.values : [0 1 3 5]             # frames 2 and 4 are absent, not NaN-padded
-     frame 0: [125. 125.]
-     frame 1: [135.  nan]                # crab 2 IS NaN-padded
-     frame 3: [155. 185.]
-     frame 5: [175. 225.]
-
-# the current default:
-use_frame_numbers_from_file=False
-  ds.time.values : [0 1 2 3]             # frame 3's data sits at time 2, frame 5's at time 3
-```
-
-### 2. `escape_state` is written by position, not by frame
-
-[Line 87](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L87)
-takes its length from the data and its index from the metadata:
-
-```python
-escape_state = np.zeros(ds.time.shape[0], dtype=np.float16)   # however many rows survived
-escape_state[local_escape_start_frame_0idx:] = 1.0            # slices frames indices in the clip --- this is the issue
-```
-
-The second line is the issue. The two lines only agree when row *i* is clip frame *i*. It could be fixed as:
-```python
-ds["escape_state"] = (ds.time >= local_escape_start_frame_0idx).astype(np.float16)
-```
-but that requires remembering that we need to use time coordinates for lookup, rather than time indices.
-
-There is a second failure mode. When `local_escape_start_frame_0idx` exceeds the number in `ds.time.shape[0]`,
-the slice assigns nothing and the escape never switches on.
-
-The repository's own test fixture does
-this: it annotates a 101-frame clip that only has 7 frames of data with an escape starting at clip frame 51. So `escape_state`
-is length 7 and `escape_state[51:] = 1.0` is a silent no-op.
-
-### 3. To make `time` dense, the two changes are jointly necessary
-
-- **`use_frame_numbers_from_file=True` alone changes nothing.** It relabels the rows; it does not add the missing ones.
-- **The reindex alone is worse than doing nothing.** The current default renumbers the
-  surviving frames `0, 1, 2, …`,. Reindexing onto the clip's range sees those labels as genuine,
-  leaves the shifted escape data where it is, and appends NaN rows at the end.
-- **Together they work.**
-  * `use_frame_numbers_from_file=True` makes the labels true frame numbers;
-  * reindexing aligns the rows to those labels and fills the holes.
-
-> [!NOTE]
-> This is about making the axis **dense**, not about the bug. The `escape_state` onset is fixed by
-> `use_frame_numbers_from_file=True` plus writing `escape_state` by label, with no reindex at all
-> (see *Rejected alternative* under [Detailed implementation](#detailed-implementation)). Whether to
-> also pad is a separate decision, argued in [§5](#5-why-time-should-be-dense).
-
-### 4. The clip's frame count comes from the metadata csv
-
-`n_clip_frames` is `loop_END_frame_ffmpeg - loop_START_frame_ffmpeg + 1`, the same expression
-[`extract_loop_clips.py`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/scripts/extract_loop_clips.py#L99-L101)
-uses to check the clips it cuts.
-
-- **The metadata csv is the only source available.** `create-zarr-dataset` is given a directory of
-  VIA tracks files and the metadata csv. It never opens the clip videos.
-- **The metadata csv is already the authority for the escape frame**, which is expressed in global
-  video frames and converted with `loop_START_frame_ffmpeg`. Taking the clip length from the same
-  row keeps one source of truth.
-- **The frame numbers in the VIA tracks file are clip-local**, because
-  [`core_detection_and_tracking`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/tracker/track_video.py#L243-L271)
-  counts `frame_idx` from 0 over the video it is given, and that video is the clip that
-  `extract-loop-clips` cut.
-
-> [!NOTE]
-> `--verify_frames` is opt-in
-> ([`extract_loop_clips.py:388-394`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/scripts/extract_loop_clips.py#L388-L394)),
-> so nothing guarantees a cut clip has exactly the frame count its metadata row states. The guard in
-> step 3 is what makes a disagreement visible.
-
-### 5. Why should `time` be dense?
-
-This is the case for padding (note that the `escape_state` bug is fixable without
-any reindex).
-
-1. **A gapped `time` is not uniformly sampled, and no user convention repairs that.** Telling people to
-slice with `.sel` rather than `.isel` fixes *lookup*; it does nothing for anything that walks the
-axis, e.g. `movement`'s own kinematics:
-
-    * `compute_forward_displacement` uses positional `.diff`, so a two-frame gap counts as one step,
-    * `compute_velocity` uses coordinate-aware `.differentiate`.
-    * Rolling windows,
-    * `n_frames / fps`
-    durations, and any speed trace around the escape onset are then silently wrong.
-
-    On a dense axis the
-    same query returns NaN for a frame with no crabs, which is the honest answer. The user does not need to keep in mind the fact that there are "missing" frames and assess whether they may have an impact or not.
-
-2. **It makes "missing" mean one thing.** `movement` already NaN-pads an individual that is absent from
-a frame (§1). A frame that *every* individual is absent from is the same fact, and dropping its label
-rather than NaN-filling it records that fact differently depending only on how many crabs happened to
-be visible. Reindexing makes the rule uniform: missing is always a NaN row, never an absent label. Less cognitive load for the user.
-
-3. The storage cost is nil.
-[`create_final_zarr_store`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L321-L327)
-already concatenates with `join="outer"`, so the stored array is dense to the *union* of the clips'
-time labels. Padding only changes that length from "frames some clip in this video had a crab" to
-"the longest clip in this video" — a handful of rows.
-
-## Detailed implementation
-
-```mermaid
-flowchart TD
-    L1["load_bboxes.from_via_tracks_file<br/>✨ use_frame_numbers_from_file=True"] --> L2
-    L2["look up the clip's row in the metadata csv"] --> L3
-    L3["✨ n_clip_frames =<br/>loop_END_frame_ffmpeg - loop_START_frame_ffmpeg + 1"] --> L4
-    L4{"✨ frame numbers within<br/>0 … n_clip_frames-1 ?<br/><i>gaps are fine</i>"}
-    L4 -- no --> X1["raise ValueError<br/><i>names the likely cause</i>"]
-    L4 -- yes --> L5["✨ ds.reindex(time=arange(n_clip_frames))"]
-    L5 --> L6{"✨ escape frame in<br/>0 … n_clip_frames-1 ?"}
-    L6 -- no --> X2["raise ValueError"]
-    L6 -- yes --> L7["escape_state = zeros(n_clip_frames)<br/>escape_state[local_escape_start:] = 1.0"]
-    L7 --> L8["expand_dims('clip_id'), clip coords<br/><i>unchanged</i>"]
-    style L1 fill:#e0e7ff,stroke:#4338ca,color:#1e1b4b
-    style L3 fill:#e0e7ff,stroke:#4338ca,color:#1e1b4b
-    style L4 fill:#e0e7ff,stroke:#4338ca,color:#1e1b4b
-    style L5 fill:#e0e7ff,stroke:#4338ca,color:#1e1b4b
-    style L6 fill:#e0e7ff,stroke:#4338ca,color:#1e1b4b
-```
-
-Arrows are the order of statements inside `load_extended_ds` after this change. ✨ marks what this
-pull request adds.
-
-Node → code: every node is inside
-[`load_extended_ds`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L49-L118);
-`L1` replaces [line 70](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L70)
-and `L7` replaces [lines 87-89](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L87-L89).
-
-Note that the "frame numbers within range" check is required because **reindex doesn't only fill in gaps — it also drops out-of-range labels**. Labels not in the new index are discarded, silently:
-```
-frames 99…199, reindex(time=arange(101))
-  → only labels 99 and 100 match; the other 99 rows are thrown away
-  → a 101-frame dataset that is NaN everywhere except two rows, no error
-
-frames 0…101, reindex(time=arange(101))
-  → frame 101's row is dropped, no warning
-```
-
-### Main changes
-
-| # | Change | Diff or signature |
-|---|---|---|
-| 1 | Ask `movement` for the frame numbers as written in the file | `from_via_tracks_file(path, use_frame_numbers_from_file=True)` |
-| 2 | Derive the clip's frame count from the metadata row | `n_clip_frames = global_clip_end_frame_0idx - global_clip_start_frame_0idx + 1` |
-| 3 | **New** guard on the frame numbers in the file | `_validate_frames_in_clip_range(...) -> None` |
-| 4 | Reindex onto the clip's frame range | `ds = ds.reindex(time=np.arange(n_clip_frames))` |
-| 5 | Size `escape_state` by the clip, and guard the escape frame | `np.zeros(n_clip_frames, dtype=np.float16)` |
-| 6 | Assert the values of `escape_state` in the unit tests | see [Tests](#tests) |
-
-<details>
-<summary><b>1-5. <code>load_extended_ds</code> in full</b></summary>
-
-The metadata row is looked up before the frame axis is settled, because the clip's frame count
-comes from that row.
-
-```diff
--    # Load VIA tracks file as movement dataset
--    ds = load_bboxes.from_via_tracks_file(via_tracks_file_path)
-+    # Load VIA tracks file as movement dataset.
-+    # We request the frame numbers as defined in the file. With the
-+    # default (use_frame_numbers_from_file=False), `movement` maps them to
-+    # a 0-based sequence of *consecutive* integers. Since the tracker
-+    # writes no rows for frames with no tracked boxes, that would shift
-+    # every subsequent frame earlier, and `time` would no longer be the
-+    # clip's frame index.
-+    ds = load_bboxes.from_via_tracks_file(
-+        via_tracks_file_path, use_frame_numbers_from_file=True
-+    )
-
-     # Extract metadata for this row
-     clip_filename = _via_tracks_to_clip_filename(ds.attrs["source_file"])
-     row = df_metadata.loc[df_metadata["loop_clip_name"] == clip_filename].iloc[
-         0
-     ]
-     global_clip_start_frame_0idx = row["loop_START_frame_ffmpeg"] - 1
-     global_clip_end_frame_0idx = row["loop_END_frame_ffmpeg"] - 1
-     global_escape_start_frame_0idx = row["escape_START_frame_0_based_idx"]
-+    n_clip_frames = (
-+        global_clip_end_frame_0idx - global_clip_start_frame_0idx + 1
-+    )
-+
-+    # Check the frames in the VIA tracks file belong to this clip
-+    _validate_frames_in_clip_range(
-+        ds.time.values,
-+        n_clip_frames,
-+        global_clip_start_frame_0idx,
-+        clip_filename,
-+    )
-+
-+    # Pad the time coordinate to span the full clip.
-+    # `movement` pads with NaNs the individuals that are missing from a
-+    # frame, but a frame that no individual appears in is absent from the
-+    # time coordinate altogether, so we reindex to add it back.
-+    n_frames_wout_boxes = n_clip_frames - ds.sizes["time"]
-+    if n_frames_wout_boxes:
-+        print(
-+            f"{clip_filename}: {n_frames_wout_boxes} of {n_clip_frames} "
-+            "frames have no tracked boxes; padding them with NaNs."
-+        )
-+    ds = ds.reindex(time=np.arange(n_clip_frames))
-
-     # Add escape_state as data variable
-     local_escape_start_frame_0idx = (
-         global_escape_start_frame_0idx - global_clip_start_frame_0idx
-     )
-+    if not 0 <= local_escape_start_frame_0idx < n_clip_frames:
-+        raise ValueError(
-+            f"{clip_filename}: the escape start frame "
-+            f"({local_escape_start_frame_0idx} as a clip frame index) is "
-+            f"outside the clip's frame range 0-{n_clip_frames - 1}."
-+        )
-     # we use float16 (not int/bool) to allow for NaN padding after
-     # concatenating along clip_id
--    escape_state = np.zeros(ds.time.shape[0], dtype=np.float16)
-+    escape_state = np.zeros(n_clip_frames, dtype=np.float16)
-     escape_state[local_escape_start_frame_0idx:] = 1.0
-     ds["escape_state"] = ("time", escape_state)
-```
-
-`print` rather than `logging`, to match the progress output the module already writes at
-[line 242](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L242).
-`np` and `Path` are already imported.
-
-Verified across five patterns of crab-free frames — none, leading, interior, two consecutive interior, and
-trailing. All give `len(time) == n_clip_frames`, the escape onset on the right frame, the crab-free
-frames present as all-NaN rows, and `ds.attrs` preserved through `reindex`.
-
-</details>
-
-<details>
-<summary><b>3. The new guard, and what it separates</b></summary>
-
-The guard sits beside the other `_`-prefixed helpers in the module.
-
-```python
-def _validate_frames_in_clip_range(
-    frames_0idx: np.ndarray,
-    n_clip_frames: int,
-    clip_first_frame_0idx: int,
-    clip_filename: str,
-) -> None:
-    """Check the frame numbers in a VIA tracks file belong to the clip.
-
-    The frame numbers are expected to be 0-based indices into the clip,
-    which is how `detect-and-track-video` numbers the frames of the clip
-    it is given.
-    """
-    if frames_0idx.min() >= 0 and frames_0idx.max() < n_clip_frames:
-        return
-
-    raise ValueError(
-        f"{clip_filename}: the VIA tracks file holds frame numbers "
-        f"{frames_0idx.min()}-{frames_0idx.max()}, outside the clip's "
-        f"frame range 0-{n_clip_frames - 1}. Frame numbers are expected "
-        f"to be 0-based indices into the clip, which starts at video "
-        f"frame {clip_first_frame_0idx}. Either the file was tracked on "
-        f"the full video or paired with the wrong metadata row, or the "
-        f"cut clip does not have the frame count the metadata csv states "
-        f"(re-run extract-loop-clips --verify_frames)."
-    )
-```
-
-One message names both causes, and the numbers in it tell them apart:
-
-| frames in the file | cause | how the numbers show it |
-|---|---|---|
-| `99..199` for a 101-frame clip starting at video frame 99 | tracked on the full video, or paired with the wrong metadata row | the range matches the clip's *global* span, not `0..100` |
-| `0..101` for a 101-frame clip | the cut clip is longer than the metadata states | the range starts at 0 and overshoots the end by a little |
-
-Measured against both cases, and against three clip-based cases (no gaps, interior gaps, trailing
-gaps) which pass.
-
-</details>
-
-<details>
-<summary><b>Rejected alternative — fix <code>escape_state</code> only, and leave the holes</b></summary>
-
-Writing `escape_state` by label rather than by position fixes the onset without any reindex:
-
-```python
-ds["escape_state"] = (ds.time >= local_escape_start_frame_0idx).astype(np.float16)
-```
-
-**It does fix the reported bug.** The onset lands on the right frame. It is rejected only because it leaves `time` non-uniformly
-sampled — [§5](#5-why-time-should-be-dense) is the whole of the case against it.
-
-It is also a smaller diff, though less so than it looks: the substantive difference is
-one line (`ds.reindex(...)`) against one line (the label comparison). Most of the rest is
-`_validate_frames_in_clip_range`, which is worth having either way — a file tracked on the full
-video, or paired with the wrong metadata row, should be an error rather than a dataset that quietly
-looks fine.
-
-Three further differences, none decisive on its own:
-
-1. **The stored time axis becomes emergent rather than stated.**
-[`create_final_zarr_store`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L321-L327)
-concatenates the clips of a video with `join="outer"`, which takes the union of their time labels. A clip missing frame 4 keeps that hole only if no sibling clip fills it:
-
-    ```
-    Loop01 alone        -> time = [0 1 2 3 5]      hole at 4
-    Loop01 with Loop03  -> time = [0 1 2 3 4 5]    Loop03 filled it
-    ```
-
-    Each array job processes one whole video, so this is deterministic for a given input directory —
-    not run-to-run flakiness. But `ds_video.time` then means "frames where some clip in this video
-    had a crab", which is a property of the detections rather than of the clips. Reindexing makes it
-    `arange(max n_clip_frames)`, which is a property of the clips. The current code cannot produce a
-    hole at all, because it renumbers every clip to `arange(n_present)` and a union of nested ranges
-    is a range.
-
-2. **It leaves `escape_state` NaN at frames where it is perfectly well defined.** The outer join
-NaN-fills every `time`-dimensioned variable, `escape_state` included:
-
-    ```
-    time                : [0 1 2 3 4 5]
-    escape_state Loop01 : [ 0.  0.  0.  1. nan  1.]     escape began at frame 3
-    ```
-
-    That overloads the NaN. The comment at
-    [lines 85-87](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L85-L87)
-    says `float16` is chosen so that clips shorter than the longest clip can be NaN-padded. A mid-clip
-    hole would make "past the end of this clip" and "no crabs were detected here" the same value.
-    Recoverable rather than lost, though: `clip_first_frame_0idx` and `clip_last_frame_0idx` are
-    stored per `clip_id`, so the two cases can always be told apart, and `escape_state` is derivable
-    from those coordinates in the first place. An annoyance, not a reason.
-
-3. **Positional indexing stops agreeing with frame numbers.** On a dense axis `isel(time=i)` and
-`sel(time=i)` are the same row, so existing code, and third-party code that indexes positionally,
-stay correct without anyone learning a convention. On a gapped axis `.isel` still runs and returns a
-plausible wrong row, and nothing detects the violation. The recipe under
-[Documentation updates](#documentation-updates) is an example: `isel(time=slice(0, n_clip_frames))`
-needs row *i* to be frame *i*; the gapped equivalent `sel(time=slice(0, n_clip_frames - 1))` runs
-fine but hands back fewer rows than the clip has frames.
-
-    This is convenience, not a checked guarantee, and it is **not** an argument that
-    `dropna(dim="time", how="all")` behaves better under padding — it does not. `dropna` cannot tell
-    "beyond this clip's end" from "no crabs in this frame", so under *both* designs it returns the
-    clip's frames *with detections*, and the assertion at
-    [`00_notebook_data_structure.py:261-266`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/notebooks/crabs_dataset/00_notebook_data_structure.py#L261-L266)
-    fails as soon as one frame is crab-free. That idiom needs replacing with the slice recipe either
-    way.
-
-</details>
-
-## Files changed
-
-| File | Change |
-|---|---|
-| [`crabs/zarr/create_dataset.py`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py) | `load_extended_ds`: anchor `time` to the clip; **new** `_validate_frames_in_clip_range` |
-| [`tests/test_unit/test_create_zarr_dataset.py`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/tests/test_unit/test_create_zarr_dataset.py) | assert the values of `escape_state` and `time`; cover both guards |
-| [`guides/CreateZarrDatasetForTracks.md`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/guides/CreateZarrDatasetForTracks.md) | state what `time` means, and that existing stores predate the fix |
-
-Nothing else is touched. In particular, no change to `create_temp_zarr_store`,
-`create_final_zarr_store`, `_renumber_individuals`, `DEFAULT_CHUNK_SIZES`, the metadata csv schema,
-or the tracker.
-
-## Tests
-
-Required. The behaviour is currently untested, and the fixture that would have caught it is the one that
-demonstrates the bug.
-
-**Keep the existing fixtures.**
-* `sample_via_tracks_file_factory`
-([`:23-52`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/tests/test_unit/test_create_zarr_dataset.py#L23-L52))
-writes frames `[0, 10, 20, 50, 60, 80, 100]` and
-* `sample_metadata_df_factory`
-([`:55-82`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/tests/test_unit/test_create_zarr_dataset.py#L55-L82))
-describes a 101-frame clip with the escape at clip frame 51. That pairing is the bug's best witness,
-so it must not be tidied into a contiguous range.
-
-Unit tests to add, all in
-[`test_create_zarr_dataset.py`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/tests/test_unit/test_create_zarr_dataset.py):
-
-1. **`escape_state` values, on the existing gappy fixture.** Fails before the change, passes after.
-
-    ```python
-    ds = load_extended_ds(via_tracks_path, df_metadata)
-    escape_state = ds.escape_state.values.ravel()
-    assert ds.sizes["time"] == 101
-    assert np.array_equal(ds.time.values, np.arange(101))
-    assert int(np.argmax(escape_state)) == 51
-    assert escape_state.sum() == 101 - 51
-    ```
-
-2. **Crab-free frames are all-NaN rows.**
-
-    Frames 1 to 9 hold no boxes in the fixture, so
-   `ds.position.isel(time=1).isnull().all()`.
-
-3. **A contiguous VIA tracks file is unaffected.**
-
-    One row per frame of the clip gives the same
-   `escape_state` as test 1, and no NaN rows.
-
-4. **The frame-range guard fires on out-of-range frame numbers.**
-
-    Parametrized over both causes: a file whose frames run `99 … 199` (video-based numbering)
-   and one whose frames run `0 … 101` (cut clip longer than the metadata states). Both raise
-   `ValueError`. Assert that the message reports the file's own range and the clip's
-   `0-100` — the numbers, not the wording.
-
-5. **The escape-frame guard fires.**
-
-    A metadata row whose `escape_START_frame_0_based_idx` falls
-   outside the clip raises `ValueError`. Today the slice silently assigns nothing.
-
-Integration: none added.
-[`test_create_temp_zarr_store`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/tests/test_unit/test_create_zarr_dataset.py#L205)
-and
-[`test_create_final_zarr_store`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/tests/test_unit/test_create_zarr_dataset.py#L269)
-already exercise the path end to end and must stay green with the same fixtures.
-
-## Documentation updates
-
-Yes, one file.
-[`guides/CreateZarrDatasetForTracks.md`](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/guides/CreateZarrDatasetForTracks.md)
-gains two lines under *Expected output*:
-
-- `time` is the clip's 0-based frame index and always spans the whole clip, with frames that hold no
-  tracked crabs stored as NaNs.
-- add a suggestion for how to get the full range of a clip (without trailing nan-padding, but with any NaN frames that are inside the clip range). E.g.
-
-  ```python
-  # Preferred option to get a clip's full range without trailing NaN-padding
-  d = ds_video.isel(clip_id=0)
-  n_clip_frames = int(d.clip_last_frame_0idx - d.clip_first_frame_0idx) + 1
-  d_clip = d.isel(time=slice(0, n_clip_frames))   # no dropna, no NaN archaeology
-  ```
-
-  and say why not `dropna(dim="time", how="all")`: it cannot tell "beyond this clip's end" from "no
-  crabs in this frame", so it silently drops crab-free frames from the middle of the clip and
-  returns a gapped axis. The slice keeps every frame of the clip and nothing else.
-
-- stores built before this change have a `time` coordinate that does not mean this, and may need
-  rebuilding.
-
-## Verifications for agent to run
-
-```bash
-# the strengthened test must fail before the change and pass after
-pytest tests/test_unit/test_create_zarr_dataset.py -v
-
-pytest tests/test_unit
-pytest tests/test_integration # run at the end, slower
-pre-commit run --all-files
-```
-
-
-**Rebuild a small store and check the invariant**, on two or three clips:
-
-```bash
-create-zarr-dataset \
-    --via_tracks_dir <dir> --metadata_csv <loop-frames-ffmpeg.csv> \
-    --zarr_store /tmp/check.zarr --zarr_mode_store w
-```
-
-```python
-import xarray as xr, numpy as np
-ds = xr.open_datatree("/tmp/check.zarr", engine="zarr", chunks={})["<video-id>"].to_dataset()
-for c in ds.clip_id.values:
-    d = ds.sel(clip_id=c)
-    expected = int(d.clip_escape_first_frame_0idx - d.clip_first_frame_0idx)
-    assert int(np.argmax(d.escape_state.values)) == expected, c
-```
-
-## Points to discuss
-
-| # | To discuss | Conclusion |
-|---|---|---|
-| 1 | **Pad the time axis, or only fix `escape_state`?** Writing `escape_state` by label corrects the onset on its own, and leaves the holes in `time`. **I recommend padding**, for the two reasons in [§5](#5-why-time-should-be-dense): a gapped axis is not uniformly sampled, so kinematics over it are silently wrong, and it records a missing frame differently from a missing individual. The three points under *Rejected alternative* are supporting, not decisive. This is worth deciding on its own terms — it is not needed to fix the bug. | We do padding |
-| 2 | **Should `movement` change too?** `from_via_tracks_file` can return a non-uniformly sampled `time` axis, and movement's own kinematics handle that inconsistently — `compute_forward_displacement` uses positional `.diff`, `compute_velocity` uses coordinate-aware `.differentiate`, so on a gapped axis they disagree about what one time step is. **I recommend filing an upstream issue** proposing that `use_frame_numbers_from_file=True` reindex to `arange(min, max+1)`. It would not remove the reindex here, because only the metadata csv knows the clip's length. Not a blocker. | I opened a PR in movement that does not fill gaps in time (in line with their current treatment of pose data), but it retains any gaps in the original VIA tracks file frame numbers if `use_frame_numbers_from_file=True`. See PR [1103](https://github.com/neuroinformatics-unit/movement/pull/1103)  |
-| 3 | **`output_video.py:176` has the same hazard.** It reads a `_tracks.csv` with `use_frame_numbers_from_file=False` and draws the boxes onto video frames, so a crab-free frame would shift the overlay. Out of scope here; worth its own issue. | Fix implemented as part of this PR |
-| 4 | **`_renumber_individuals` discards the original track IDs** ([lines 193-200](https://github.com/SainsburyWellcomeCentre/crabs-exploration/blob/main/crabs/zarr/create_dataset.py#L193-L200)), resetting each clip's individuals to `id_0 … id_{N-1}`. That makes the store unusable as a source of tracker track IDs. Deliberate for the escape analysis, but worth recording. Out of scope. | We don't care about retaining the exact ID returned by the tracker. We just care about them being "distinct" as the tracker defined them (regardless of their exact name). We force them to be consecutive and monotonically increasing to keep the `individual` axis compact in the zarr store. So that can be left as is.|

From 57e0612c14e091997736cd33c6126d18792bd7d6 Mon Sep 17 00:00:00 2001
From: sfmig <33267254+sfmig@users.noreply.github.com>
Date: Wed, 16 Sep 2026 17:41:09 +0100
Subject: [PATCH 11/19] Review tests

---
 tests/test_unit/test_create_zarr_dataset.py | 147 +++++++++++---------
 1 file changed, 83 insertions(+), 64 deletions(-)

diff --git a/tests/test_unit/test_create_zarr_dataset.py b/tests/test_unit/test_create_zarr_dataset.py
index 643432b7..c4630616 100644
--- a/tests/test_unit/test_create_zarr_dataset.py
+++ b/tests/test_unit/test_create_zarr_dataset.py
@@ -20,6 +20,9 @@
     load_extended_ds,
 )
 
+# Frames the sample VIA tracks file holds boxes for by default
+DEFAULT_FRAMES_WITH_BOXES = [0, 10, 20, 50, 60, 80, 100]
+
 
 @pytest.fixture
 def sample_via_tracks_file_factory(tmp_path):
@@ -28,13 +31,14 @@ def sample_via_tracks_file_factory(tmp_path):
     def _sample_via_tracks_file(filename, frame_indices=None):
         """Create a mock VIA tracks file.
 
-        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 few frames in a 101-frame
+        clip ("sparse" case). Pass `frame_indices` to write boxes
+        for a different set of frames, e.g. `range(101)` for the "dense"
+        case of one row per frame of the clip.
         """
         # Add a few frames with bounding box annotations
         via_data = []
-        for frame_idx in frame_indices or [0, 10, 20, 50, 60, 80, 100]:
+        for frame_idx in frame_indices or DEFAULT_FRAMES_WITH_BOXES:
             via_data.append(
                 {
                     "filename": f"frame_{frame_idx:08d}.png",
@@ -128,21 +132,41 @@ def test_group_files_per_video(tmp_path):
     assert len(map_videos_to_files["04.09.2023-02-Right"]) == 1
 
 
+@pytest.mark.parametrize(
+    "via_frame_indices, expected_frames_in_ds",
+    [
+        (
+            None,  # use default sparse frames from fixture
+            DEFAULT_FRAMES_WITH_BOXES,
+        ),
+        (
+            list(range(101)),
+            list(range(101)),
+        ),
+    ],
+    ids=[
+        "sparse_file",
+        "dense_file",
+    ],
+)
 def test_load_extended_ds(
-    sample_via_tracks_file_factory, sample_metadata_df_factory
+    via_frame_indices,
+    expected_frames_in_ds,
+    sample_via_tracks_file_factory,
+    sample_metadata_df_factory,
 ):
-    """Test loading VIA tracks with metadata as a `movement` dataset."""
-    # Sample VIA tracks file
+    """Test loading VIA tracks with metadata as an extended dataset."""
+    # Get sample VIA tracks file
     via_tracks_filename = "04.09.2023-01-Right-Loop05_tracks.csv"
-    via_tracks_path = sample_via_tracks_file_factory(via_tracks_filename)
+    via_tracks_path = sample_via_tracks_file_factory(
+        via_tracks_filename,
+        frame_indices=via_frame_indices,
+    )
 
-    # Define metadata df that contains the clip for the VIA tracks file
-    # list_clip_filepath = [
-    #     Path(str(via_tracks_path).replace("_tracks.csv", ".mp4"))
-    # ]
+    # Define metadata df for the VIA tracks file
     df_metadata = sample_metadata_df_factory([via_tracks_path])
 
-    # Load dataset for this clip chunked
+    # Load extended dataset for this clip
     ds = load_extended_ds(via_tracks_path, df_metadata)
 
     # Check escape_state is a data var
@@ -162,90 +186,85 @@ def test_load_extended_ds(
     assert np.array_equal(ds.time.values, np.arange(101))
 
     # Check the escape starts at clip frame 51
-    # (global frame 150, clip starting at global frame 99)
+    # the escape is at global frame 150 (0-based) and
+    # the clip starts at global frame 99
+    # (loop_START_frame_ffmpeg=100, 1-indexed),
+    # so 150 - 99 = 51
     escape_state = ds.escape_state.values.ravel()
     assert int(np.argmax(escape_state)) == 51
     assert escape_state.sum() == 101 - 51
 
-    # Check frames with no boxes are all-NaN rows, rather than missing
-    # (the VIA tracks file holds boxes for frames 0, 10, 20, 50, 60, 80, 100)
-    assert ds.position.isel(time=1).isnull().all()
-    assert not ds.position.isel(time=10).isnull().any()
-
-
-def test_load_extended_ds_contiguous_file(
-    sample_via_tracks_file_factory, sample_metadata_df_factory
-):
-    """Test a VIA tracks file with one row per frame of the clip.
-
-    It should give the same `time` and `escape_state` as the sparse file,
-    and no NaN rows.
-    """
-    via_tracks_path = sample_via_tracks_file_factory(
-        "04.09.2023-01-Right-Loop05_tracks.csv",
-        frame_indices=list(range(101)),
+    # Check frames with no boxes in VIA file are all-NaN in the
+    # extended dataset
+    frames_not_nan = (
+        ds.position.notnull()
+        .all(dim=["clip_id", "space", "individual"])
+        .values
+    )
+    assert np.array_equal(
+        np.flatnonzero(frames_not_nan),
+        expected_frames_in_ds,
     )
-    df_metadata = sample_metadata_df_factory([via_tracks_path])
-
-    ds = load_extended_ds(via_tracks_path, df_metadata)
-
-    assert np.array_equal(ds.time.values, np.arange(101))
-    escape_state = ds.escape_state.values.ravel()
-    assert int(np.argmax(escape_state)) == 51
-    assert escape_state.sum() == 101 - 51
-    assert not ds.position.isnull().any()
 
 
-@pytest.mark.parametrize(
-    "frame_indices",
-    [
-        pytest.param(
-            list(range(99, 200)),
-            id="tracked_on_full_video_or_wrong_metadata_row",
-        ),
-        pytest.param(
-            list(range(102)), id="cut_clip_longer_than_metadata_states"
-        ),
-    ],
-)
-def test_load_extended_ds_frames_out_of_clip_range(
-    frame_indices, sample_via_tracks_file_factory, sample_metadata_df_factory
+def test_frames_in_via_out_of_clip_range(
+    sample_via_tracks_file_factory,
+    sample_metadata_df_factory,
 ):
-    """Test the guard on frame numbers outside the clip's frame range.
+    """Test the guard on VIA frame numbers outside the clip's frame range.
 
-    Without it, `reindex` would silently drop the out-of-range rows.
+    Frame numbers in the VIA file are expected to be 0-based indices into
+    the clip.
     """
+    # Get VIA tracks file with one frame more than the clip
+    # (the clip frame indices range 0 to 100)
+    via_frame_indices = list(range(102))  # clip-based indices
     via_tracks_path = sample_via_tracks_file_factory(
-        "04.09.2023-01-Right-Loop05_tracks.csv", frame_indices=frame_indices
+        "04.09.2023-01-Right-Loop05_tracks.csv",
+        frame_indices=via_frame_indices,
     )
+    # Get clip metadata; it assumes the clip spans global frames 99-199,
+    # (i.e. clip frames 0-100)
     df_metadata = sample_metadata_df_factory([via_tracks_path])
 
+    # Check ValueError is thrown
     with pytest.raises(ValueError) as exc_info:
         load_extended_ds(via_tracks_path, df_metadata)
 
-    # The message reports the file's own range and the clip's range
+    # Check message reports the file's own range and the clip's range
     message = str(exc_info.value)
-    assert f"{frame_indices[0]}-{frame_indices[-1]}" in message
+    assert f"{via_frame_indices[0]}-{via_frame_indices[-1]}" in message
     assert "0-100" in message
 
 
-def test_load_extended_ds_escape_frame_out_of_clip_range(
+def test_escape_frame_out_of_clip_range(
     sample_via_tracks_file_factory, sample_metadata_df_factory
 ):
-    """Test the guard on an escape frame outside the clip's frame range."""
+    """Test the guard on an escape frame outside the clip's frame range.
+
+    It should be checked in load_extended_ds
+    """
+    # Get VIA tracks file and metadata
     via_tracks_path = sample_via_tracks_file_factory(
         "04.09.2023-01-Right-Loop05_tracks.csv"
     )
     df_metadata = sample_metadata_df_factory([via_tracks_path])
 
-    # Clip spans global frames 99-199; put the escape past its end
+    # Place escape past the clip end.
+    # escape_START_frame_0_based_idx is in global (whole-video) numbering,
+    # and the clip spans global frames 99-199, so we place it at
+    # global frame 250
     df_metadata.loc[0, "escape_START_frame_0_based_idx"] = 250
 
+    # Check ValueError is thrown
     with pytest.raises(ValueError) as exc_info:
         load_extended_ds(via_tracks_path, df_metadata)
 
-    assert "escape start frame" in str(exc_info.value)
-    assert "0-100" in str(exc_info.value)
+    # Check error message reports the clip-local index and range.
+    message = str(exc_info.value)
+    assert "escape start frame" in message
+    assert "151 as a clip frame index" in message
+    assert "0-100" in message
 
 
 @pytest.mark.parametrize(

From 4aa31374e852911741877d154e107c6f2c4017cc Mon Sep 17 00:00:00 2001
From: sfmig <33267254+sfmig@users.noreply.github.com>
Date: Wed, 16 Sep 2026 17:42:49 +0100
Subject: [PATCH 12/19] Apply batched suggestions from code review

Co-authored-by: sfmig <33267254+sfmig@users.noreply.github.com>
Signed-off-by: sfmig <33267254+sfmig@users.noreply.github.com>
---
 crabs/zarr/create_dataset.py          | 37 +++++++++++----------------
 guides/CreateZarrDatasetForTracks.md  | 14 +++++-----
 scripts/visualization/output_video.py | 27 +++++++------------
 3 files changed, 31 insertions(+), 47 deletions(-)

diff --git a/crabs/zarr/create_dataset.py b/crabs/zarr/create_dataset.py
index 1c828fbe..33aeae06 100644
--- a/crabs/zarr/create_dataset.py
+++ b/crabs/zarr/create_dataset.py
@@ -67,12 +67,8 @@ def load_extended_ds(
 
     """
     # Load VIA tracks file as movement dataset.
-    # We request the frame numbers as defined in the file. With the
-    # default (use_frame_numbers_from_file=False), `movement` maps them to
-    # a 0-based sequence of *consecutive* integers. Since the tracker
-    # writes no rows for frames with no tracked boxes, that would shift
-    # every subsequent frame earlier, and `time` would no longer be the
-    # clip's frame index.
+    # We request the frame numbers as defined in the file. Note that
+    # the tracker only writes frames with tracked boxes.
     ds = load_bboxes.from_via_tracks_file(
         via_tracks_file_path, use_frame_numbers_from_file=True
     )
@@ -89,7 +85,8 @@ def load_extended_ds(
         global_clip_end_frame_0idx - global_clip_start_frame_0idx + 1
     )
 
-    # Check the frames in the VIA tracks file belong to this clip
+    # Check the frames in the VIA tracks file are all
+    # within range
     _validate_frames_in_clip_range(
         ds.time.values,
         n_clip_frames,
@@ -98,15 +95,14 @@ def load_extended_ds(
     )
 
     # Pad the time coordinate to span the full clip.
-    # `movement` pads with NaNs the individuals that are missing from a
-    # frame, but a frame that no individual appears in is absent from the
-    # time coordinate altogether, so we reindex to add it back.
     n_frames_wout_boxes = n_clip_frames - ds.sizes["time"]
     if n_frames_wout_boxes:
         print(
             f"{clip_filename}: {n_frames_wout_boxes} of {n_clip_frames} "
             "frames have no tracked boxes; padding them with NaNs."
         )
+	# Reindex time coordinates to fill in empy frames with NaN
+	# (out-of-range frames are silently dropped, hence the range check above)
     ds = ds.reindex(time=np.arange(n_clip_frames))
 
     # Add escape_state as data variable
@@ -115,9 +111,9 @@ def load_extended_ds(
     )
     if not 0 <= local_escape_start_frame_0idx < n_clip_frames:
         raise ValueError(
-            f"{clip_filename}: the escape start frame "
+            f"{clip_filename}: the escape start frame index "
             f"({local_escape_start_frame_0idx} as a clip frame index) is "
-            f"outside the clip's frame range 0-{n_clip_frames - 1}."
+            f"outside the clip's range 0-{n_clip_frames - 1}."
         )
     # we use float16 (not int/bool) to allow for NaN padding after
     # concatenating along clip_id
@@ -161,24 +157,21 @@ def _validate_frames_in_clip_range(
     clip_first_frame_0idx: int,
     clip_filename: str,
 ) -> None:
-    """Check the frame numbers in a VIA tracks file belong to the clip.
+    """Check the frame numbers in a VIA tracks file are within the clip frame range.
 
-    The frame numbers are expected to be 0-based indices into the clip,
-    which is how `detect-and-track-video` numbers the frames of the clip
-    it is given.
+    The frame indices are 0-based indices into the clip.
     """
     if frames_0idx.min() >= 0 and frames_0idx.max() < n_clip_frames:
         return
 
     raise ValueError(
-        f"{clip_filename}: the VIA tracks file holds frame numbers "
+        f"{clip_filename}: the VIA tracks file holds frame indices "
         f"{frames_0idx.min()}-{frames_0idx.max()}, outside the clip's "
-        f"frame range 0-{n_clip_frames - 1}. Frame numbers are expected "
+        f"range 0-{n_clip_frames - 1}. Frame indices are expected "
         f"to be 0-based indices into the clip, which starts at video "
-        f"frame {clip_first_frame_0idx}. Either the file was tracked on "
-        f"the full video or paired with the wrong metadata row, or the "
-        f"cut clip does not have the frame count the metadata csv states "
-        f"(re-run extract-loop-clips --verify_frames)."
+        f"frame {clip_first_frame_0idx}. Either the frames in the file refer to "
+        f"the full video, the file is paired with the wrong metadata row, or the "
+        f"clip does not have the frame count the metadata csv states. "
     )
 
 
diff --git a/guides/CreateZarrDatasetForTracks.md b/guides/CreateZarrDatasetForTracks.md
index fafb5d19..687a1c84 100644
--- a/guides/CreateZarrDatasetForTracks.md
+++ b/guides/CreateZarrDatasetForTracks.md
@@ -80,20 +80,20 @@
 
     If the array job runs successfully, a zarr dataset named `CrabTracks-slurm<SLURM_ARRAY_JOB_ID>.zarr` will be generated in the location specified by `ZARR_STORE_OUTPUT`. Each group in the zarr dataset will correspond to a `movement` [bounding box dataset](https://movement.neuroinformatics.dev/latest/user_guide/movement_dataset.html) containing all tracks for one video.
 
-    The `time` coordinate is the clip's 0-based frame index, and it always spans the whole clip: frames that hold no tracked crabs are stored as NaNs rather than dropped. So within a clip, row `i` is frame `i`.
+    The `time` coordinate is the clip's 0-based frame index, and it always spans the whole clip. Frames that hold no tracked crabs are stored as NaNs. So within a clip, index `i` is frame `i`.
 
-    To get a single clip's full frame range, without the NaN padding that the concatenation along `clip_id` adds beyond the end of shorter clips:
+    To get a single clip's full frame range, without the trailing NaN padding that the concatenation along `clip_id` adds:
 
     ```python
-    d = ds_video.isel(clip_id=0)
-    n_clip_frames = int(d.clip_last_frame_0idx - d.clip_first_frame_0idx) + 1
-    d_clip = d.isel(time=slice(0, n_clip_frames))
+    ds = ds_video.isel(clip_id=0)
+    n_clip_frames = int(ds.clip_last_frame_0idx - ds.clip_first_frame_0idx) + 1
+    ds_clip = ds.isel(time=slice(0, n_clip_frames)) # or equivalently: ds.sel(time=slice(0, n_clip_frames - 1)). Note that .sel is end-inclusive.
     ```
 
-    Prefer this to `dropna(dim="time", how="all")`, which cannot tell "beyond this clip's end" from "no crabs in this frame": it silently drops crab-free frames from the middle of the clip and returns a gapped time axis. The slice keeps every frame of the clip and nothing else.
+    Notice that when we drop all NaN frames with `dropna(dim="time", how="all")`, we cannot tell which NaN frames are padding and which NaN frames are "no detections in this frame": it silently returns a potentially gapped time axis. Instead the slice keeps every frame of the clip.
 
     > [!WARNING]
-    > Zarr stores built before the `time` coordinate was anchored to the clip have a `time` coordinate that does not mean this. There, `time` was a 0-based count of the frames *with detections*, so frames with no tracked crabs shifted every later frame earlier, and `escape_state` switched on at the wrong frame. Those stores may need rebuilding.
+    > Zarr stores built before the `time` coordinate was made dense have a `time` coordinate that does not mean this. There, `time` was a 0-based count of the frames *with detections*, so frames with no tracked crabs shifted every later frame earlier, and `escape_state` switched on at the wrong frame. Those stores may need rebuilding if there were frames with no detections in them.
 
     To inspect this dataset, please refer to the example notebooks in the 🦀 repository.
 
diff --git a/scripts/visualization/output_video.py b/scripts/visualization/output_video.py
index 4f9b7351..1db9b864 100644
--- a/scripts/visualization/output_video.py
+++ b/scripts/visualization/output_video.py
@@ -36,33 +36,24 @@ def get_distinct_colors():
 
 
 def reindex_to_video_frames(ds, video_path):
-    """Pad the dataset's time coordinate to span every frame of the video.
-
-    `movement` pads with NaNs the individuals that are missing from a frame,
-    but a frame that no individual appears in is absent from the time
-    coordinate altogether. We reindex to add those frames back, so that row
-    `i` of the dataset is frame `i` of the video and the arrays can be
-    indexed positionally by frame number.
-
-    The dataset is expected to hold the frame numbers as written in the VIA
-    tracks file (i.e. loaded with `use_frame_numbers_from_file=True` and
-    `fps=None`), which the tracker numbers from 0 over the video it is given.
+    """Pad the dataset's time coordinate to span every frame of the clip.
+    
+    The dataset is expected to hold the frame numbers 0-based indices over
+    the clip, as written in the VIA tracks file (i.e. loaded with `use_frame_numbers_from_file=True` and `fps=None`).
     """
-    # Get n frames in video
+    # Get n frames in video clip
     cap = cv2.VideoCapture(str(video_path))
     n_frames = int(cap.get(cv2.CAP_PROP_FRAME_COUNT))
     cap.release()
 
-    # Check the frame numbers belong to this video. `reindex` discards
-    # out-of-range labels silently, so without this an unrelated tracks file
-    # would give an all-NaN dataset rather than an error.
+    # Check the frame indices in the file are within the video range
     frames_0idx = ds.time.values
     if frames_0idx.min() < 0 or frames_0idx.max() >= n_frames:
         raise ValueError(
             f"{Path(video_path).name}: the frame numbers in the tracks file "
             f"({frames_0idx.min()}-{frames_0idx.max()}) fall outside the "
             f"video's frame range 0-{n_frames - 1}. Are the tracks from this "
-            "video?"
+            "clip?"
         )
 
     # Print number of frames with no detections
@@ -226,8 +217,8 @@ def create_opencv_video(
             use_frame_numbers_from_file=True,
         )
 
-        # Pad the time coordinate to span the full video, so that row i of
-        # the dataset is frame i of the video
+        # Pad the time coordinate to span the full video, so that 
+        # time coordinate i of the dataset is frame index (0-based) i of the clip
         ds_pred = reindex_to_video_frames(ds_pred, input_video)
 
         list_individuals_idcs = list(range(len(ds_pred.individuals)))

From 0ed7597ef988374f1b68bbe7489cc57898215a7e Mon Sep 17 00:00:00 2001
From: "pre-commit-ci[bot]"
 <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Date: Wed, 16 Sep 2026 16:43:07 +0000
Subject: [PATCH 13/19] [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci
---
 crabs/zarr/create_dataset.py          | 4 ++--
 scripts/visualization/output_video.py | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/crabs/zarr/create_dataset.py b/crabs/zarr/create_dataset.py
index 33aeae06..0dc3f1b6 100644
--- a/crabs/zarr/create_dataset.py
+++ b/crabs/zarr/create_dataset.py
@@ -101,8 +101,8 @@ def load_extended_ds(
             f"{clip_filename}: {n_frames_wout_boxes} of {n_clip_frames} "
             "frames have no tracked boxes; padding them with NaNs."
         )
-	# Reindex time coordinates to fill in empy frames with NaN
-	# (out-of-range frames are silently dropped, hence the range check above)
+    # Reindex time coordinates to fill in empy frames with NaN
+    # (out-of-range frames are silently dropped, hence the range check above)
     ds = ds.reindex(time=np.arange(n_clip_frames))
 
     # Add escape_state as data variable
diff --git a/scripts/visualization/output_video.py b/scripts/visualization/output_video.py
index 1db9b864..3239da1c 100644
--- a/scripts/visualization/output_video.py
+++ b/scripts/visualization/output_video.py
@@ -37,7 +37,7 @@ def get_distinct_colors():
 
 def reindex_to_video_frames(ds, video_path):
     """Pad the dataset's time coordinate to span every frame of the clip.
-    
+
     The dataset is expected to hold the frame numbers 0-based indices over
     the clip, as written in the VIA tracks file (i.e. loaded with `use_frame_numbers_from_file=True` and `fps=None`).
     """
@@ -217,7 +217,7 @@ def create_opencv_video(
             use_frame_numbers_from_file=True,
         )
 
-        # Pad the time coordinate to span the full video, so that 
+        # Pad the time coordinate to span the full video, so that
         # time coordinate i of the dataset is frame index (0-based) i of the clip
         ds_pred = reindex_to_video_frames(ds_pred, input_video)
 

From b20586ffc0ce78f554fb6580cd1a98b8c0895312 Mon Sep 17 00:00:00 2001
From: sfmig <33267254+sfmig@users.noreply.github.com>
Date: Thu, 17 Sep 2026 14:11:41 +0100
Subject: [PATCH 14/19] Wrap lines that exceeded the 79-char limit

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>
---
 crabs/zarr/create_dataset.py          | 9 +++++----
 scripts/visualization/output_video.py | 7 ++++---
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/crabs/zarr/create_dataset.py b/crabs/zarr/create_dataset.py
index 0dc3f1b6..6e519241 100644
--- a/crabs/zarr/create_dataset.py
+++ b/crabs/zarr/create_dataset.py
@@ -157,7 +157,7 @@ def _validate_frames_in_clip_range(
     clip_first_frame_0idx: int,
     clip_filename: str,
 ) -> None:
-    """Check the frame numbers in a VIA tracks file are within the clip frame range.
+    """Check the frame numbers in a VIA tracks file are within the clip.
 
     The frame indices are 0-based indices into the clip.
     """
@@ -169,9 +169,10 @@ def _validate_frames_in_clip_range(
         f"{frames_0idx.min()}-{frames_0idx.max()}, outside the clip's "
         f"range 0-{n_clip_frames - 1}. Frame indices are expected "
         f"to be 0-based indices into the clip, which starts at video "
-        f"frame {clip_first_frame_0idx}. Either the frames in the file refer to "
-        f"the full video, the file is paired with the wrong metadata row, or the "
-        f"clip does not have the frame count the metadata csv states. "
+        f"frame {clip_first_frame_0idx}. Either the frames in the file "
+        f"refer to the full video, the file is paired with the wrong "
+        f"metadata row, or the clip does not have the frame count the "
+        f"metadata csv states. "
     )
 
 
diff --git a/scripts/visualization/output_video.py b/scripts/visualization/output_video.py
index 3239da1c..bda329a6 100644
--- a/scripts/visualization/output_video.py
+++ b/scripts/visualization/output_video.py
@@ -39,7 +39,8 @@ def reindex_to_video_frames(ds, video_path):
     """Pad the dataset's time coordinate to span every frame of the clip.
 
     The dataset is expected to hold the frame numbers 0-based indices over
-    the clip, as written in the VIA tracks file (i.e. loaded with `use_frame_numbers_from_file=True` and `fps=None`).
+    the clip, as written in the VIA tracks file (i.e. loaded with
+    `use_frame_numbers_from_file=True` and `fps=None`).
     """
     # Get n frames in video clip
     cap = cv2.VideoCapture(str(video_path))
@@ -217,8 +218,8 @@ def create_opencv_video(
             use_frame_numbers_from_file=True,
         )
 
-        # Pad the time coordinate to span the full video, so that
-        # time coordinate i of the dataset is frame index (0-based) i of the clip
+        # Pad the time coordinate to span the full video, so that time
+        # coordinate i of the dataset is frame index (0-based) i of the clip
         ds_pred = reindex_to_video_frames(ds_pred, input_video)
 
         list_individuals_idcs = list(range(len(ds_pred.individuals)))

From eb4c2806c8d5a997e6f5e45be2df9beda8f45cbe Mon Sep 17 00:00:00 2001
From: sfmig <33267254+sfmig@users.noreply.github.com>
Date: Thu, 17 Sep 2026 15:21:51 +0100
Subject: [PATCH 15/19] Small edits

---
 guides/CreateZarrDatasetForTracks.md  | 2 +-
 scripts/visualization/output_video.py | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/guides/CreateZarrDatasetForTracks.md b/guides/CreateZarrDatasetForTracks.md
index 687a1c84..3c53af7f 100644
--- a/guides/CreateZarrDatasetForTracks.md
+++ b/guides/CreateZarrDatasetForTracks.md
@@ -82,7 +82,7 @@
 
     The `time` coordinate is the clip's 0-based frame index, and it always spans the whole clip. Frames that hold no tracked crabs are stored as NaNs. So within a clip, index `i` is frame `i`.
 
-    To get a single clip's full frame range, without the trailing NaN padding that the concatenation along `clip_id` adds:
+    To get a single clip's full frame range from a video dataset, without the trailing NaN padding that the concatenation along `clip_id` adds:
 
     ```python
     ds = ds_video.isel(clip_id=0)
diff --git a/scripts/visualization/output_video.py b/scripts/visualization/output_video.py
index bda329a6..f42d0100 100644
--- a/scripts/visualization/output_video.py
+++ b/scripts/visualization/output_video.py
@@ -35,7 +35,7 @@ def get_distinct_colors():
     return colors
 
 
-def reindex_to_video_frames(ds, video_path):
+def reindex_to_video_clip_frames(ds, video_path):
     """Pad the dataset's time coordinate to span every frame of the clip.
 
     The dataset is expected to hold the frame numbers 0-based indices over
@@ -206,7 +206,7 @@ def create_opencv_video(
         ]
     )
     for escape_clip_name in list_escape_clips[-1:]:
-        input_video = Path(input_data_dir) / f"{escape_clip_name}.mp4"
+        input_clip = Path(input_data_dir) / f"{escape_clip_name}.mp4"
         pred_csv = Path(input_data_dir) / f"{escape_clip_name}_tracks.csv"
 
         # Read predictions as a movement dataset.
@@ -220,7 +220,7 @@ def create_opencv_video(
 
         # Pad the time coordinate to span the full video, so that time
         # coordinate i of the dataset is frame index (0-based) i of the clip
-        ds_pred = reindex_to_video_frames(ds_pred, input_video)
+        ds_pred = reindex_to_video_clip_frames(ds_pred, input_clip)
 
         list_individuals_idcs = list(range(len(ds_pred.individuals)))
 
@@ -230,7 +230,7 @@ def create_opencv_video(
         )
         create_opencv_video(
             ds=ds_pred,
-            input_video=input_video,
+            input_video=input_clip,
             output_video_path=output_video_path,
             list_individuals_idcs=list_individuals_idcs,
         )

From 9a57dd1a5c3b929a1e8dd380658961bebb00b352 Mon Sep 17 00:00:00 2001
From: sfmig <33267254+sfmig@users.noreply.github.com>
Date: Fri, 18 Sep 2026 13:16:40 +0100
Subject: [PATCH 16/19] Check http?

---
 tests/fixtures/integration.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/fixtures/integration.py b/tests/fixtures/integration.py
index 98439557..e0020a7d 100644
--- a/tests/fixtures/integration.py
+++ b/tests/fixtures/integration.py
@@ -5,7 +5,9 @@
 import pooch
 import pytest
 
-GIN_TEST_DATA_REPO = "https://gin.g-node.org/SainsburyWellcomeCentre/crabs-exploration-test-data"
+GIN_TEST_DATA_REPO = (
+    "http://gin.g-node.org/SainsburyWellcomeCentre/crabs-exploration-test-data"
+)
 
 
 @pytest.fixture(scope="session")

From d09372d41160e72e88054d14880a3deb7df6b6d3 Mon Sep 17 00:00:00 2001
From: sfmig <33267254+sfmig@users.noreply.github.com>
Date: Fri, 18 Sep 2026 15:24:53 +0100
Subject: [PATCH 17/19] Revert "Check http?"

This reverts commit 9a57dd1a5c3b929a1e8dd380658961bebb00b352.
---
 tests/fixtures/integration.py | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tests/fixtures/integration.py b/tests/fixtures/integration.py
index e0020a7d..98439557 100644
--- a/tests/fixtures/integration.py
+++ b/tests/fixtures/integration.py
@@ -5,9 +5,7 @@
 import pooch
 import pytest
 
-GIN_TEST_DATA_REPO = (
-    "http://gin.g-node.org/SainsburyWellcomeCentre/crabs-exploration-test-data"
-)
+GIN_TEST_DATA_REPO = "https://gin.g-node.org/SainsburyWellcomeCentre/crabs-exploration-test-data"
 
 
 @pytest.fixture(scope="session")

From cab4ceb4b3b2d5e2d9fe352cb6de4fd5e8209388 Mon Sep 17 00:00:00 2001
From: sfmig <33267254+sfmig@users.noreply.github.com>
Date: Fri, 18 Sep 2026 15:26:28 +0100
Subject: [PATCH 18/19] Try swc gin

---
 tests/fixtures/integration.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/fixtures/integration.py b/tests/fixtures/integration.py
index 98439557..9449faf1 100644
--- a/tests/fixtures/integration.py
+++ b/tests/fixtures/integration.py
@@ -5,7 +5,9 @@
 import pooch
 import pytest
 
-GIN_TEST_DATA_REPO = "https://gin.g-node.org/SainsburyWellcomeCentre/crabs-exploration-test-data"
+GIN_TEST_DATA_REPO = (
+    "https://swc.ucl.ac.uk/SainsburyWellcomeCentre/crabs-exploration-test-data"
+)
 
 
 @pytest.fixture(scope="session")

From d8289a9f21183b821055adbfaf567fc4ff4606f2 Mon Sep 17 00:00:00 2001
From: sfmig <33267254+sfmig@users.noreply.github.com>
Date: Fri, 18 Sep 2026 16:23:05 +0100
Subject: [PATCH 19/19] Fix url

---
 tests/fixtures/integration.py | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tests/fixtures/integration.py b/tests/fixtures/integration.py
index 9449faf1..7c1a8972 100644
--- a/tests/fixtures/integration.py
+++ b/tests/fixtures/integration.py
@@ -5,9 +5,7 @@
 import pooch
 import pytest
 
-GIN_TEST_DATA_REPO = (
-    "https://swc.ucl.ac.uk/SainsburyWellcomeCentre/crabs-exploration-test-data"
-)
+GIN_TEST_DATA_REPO = "https://gin.swc.ucl.ac.uk/SainsburyWellcomeCentre/crabs-exploration-test-data"
 
 
 @pytest.fixture(scope="session")