Skip to content

docs: ADR 0002, a unified pose file format - #445

Open
bergsalex wants to merge 5 commits into
mainfrom
docs/adr-0002-pose-format
Open

docs: ADR 0002, a unified pose file format#445
bergsalex wants to merge 5 commits into
mainfrom
docs/adr-0002-pose-format

Conversation

@bergsalex

Copy link
Copy Markdown
Collaborator

Proposes one self-describing pose file to replace pose_est_v2 through v8, with a complete revision-1 specification: root identification, a JSON manifest with its JSON Schema, the component catalogue, encodings, provenance, attachments and the validation rules.

The organising idea is that a reader asks what a file contains, never how old it is. A manifest declares each component's axes, dtype, units, coordinate order and missing-value policy, so evolution is additive and data JABS does not understand can be subset and validated by tools that know nothing about its meaning.

Two decisions carry most of the benefit. Slot k means identity k, which retires the scatter-and-flip currently reimplemented in pose_est_v4.py, the cache writer and JABS-postprocess's clip_utils.py, and with it the per-video cache file. And absence is always stated -- NaN or an explicit mask or length, never a sentinel -- which removes the 0-is-both-padding- and-a-valid-pixel ambiguity that makes every consumer re-derive validity.

Measured rather than assumed: instance_embedding is 5.19 MB per mouse-hour that nothing reads, while identity_embeds and instance_id_center look equally vestigial from inside JABS but are read by JABS-postprocess for cross-video linking. seg_data is 99.2% padding in the v8 case, though ragged encoding would not shrink the file -- gzip already removes the padding -- so dense stays the baseline.

The schema in this document was checked: it is valid draft-2020-12 and the example manifest validates against it.

@bergsalex
bergsalex requested a review from gbeane September 1, 2026 13:41
@bergsalex bergsalex self-assigned this Sep 1, 2026
@bergsalex
bergsalex marked this pull request as ready for review September 1, 2026 13:58

### What disappears

- The per-video `*_cache.h5` file and `pose_attribute_cache.json` — both memoize transforms the

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

identity_mask in JABS is not an occupancy flag, so I think this bullet and the instance_count one below it (line 762) don't hold as written.

pose_est_v4.py:165-172 builds it as:

# require a minimum number of points to be > 3
# this is because the convex hull requires 3 points
lambda x, y: np.sum(self._point_mask[x][y][:-2]) >= 3

Two things are stacked in there. _point_mask is confidence > MINIMUM_CONFIDENCE (0.3), so it is a quality gate — "are there enough confidently-detected keypoints to compute a convex hull?" — rather than "did inference put an instance in this slot?". And [:-2] excludes MID_TAIL and TIP_TAIL, so only the 10 body keypoints count.

That disagrees with a producer occupancy flag on a real class of frames:

frame state identity_present JABS identity_mask
2 confident body keypoints true false
5 body keypoints, all conf 0.2 true false
tail only, high confidence true false

Which affects two conclusions:

  • instance_count as the row-sum of identity_present is right for occupancy, but that isn't the count JABS uses anywhere.
  • The *_cache.h5 file. _cache_poses (pose_est_v4.py:311-355) writes exactly three datasets: points, point_mask, identity_mask. The first two are re-derivable from an identity-aligned format, so those really do become free. identity_mask isn't — no producer computes the >=3 non-tail keypoints above 0.3 rule, because it encodes a JABS consumer requirement rather than a property of the data.

Worth noting how load-bearing that array is: it becomes _frame_valid in features.py:335, and gates centroid velocity, lixit features, every window operation, and label loading (parallel_workers.py:352 forces labels to NONE where it is false). A migration that mapped identity_mask -> identity_present would quietly admit degenerate frames into hull and segmentation features and change which frames are trainable.

Three ways out, and I think the ADR should pick one rather than leave the mapping implied:

  1. Consumer concern. identity_present stays pure occupancy; JABS derives its own mask from point_valid at load. Honest, but then the cache-elimination claim needs qualifying.
  2. Declare it. A jabs.pose.identity_usable component with the rule (threshold, >=3, tail exclusion) recorded in provenance.
  3. Producer writes both — occupancy plus a declared quality mask with its parameters.

(2) feels most in the spirit of the document: it is the same argument you make about MINIMUM_CONFIDENCE being re-derived by every consumer. The threshold, the >=3, and the tail exclusion are precisely the sort of undeclared policy the manifest is meant to capture.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Verified all of it, and the correction stands — two bullets in "What disappears" were wrong.

pose_est_v4.py:159-172 is as you describe: _point_mask = confidence_by_id > MINIMUM_CONFIDENCE, then sum(_point_mask[x][y][:-2]) >= 3. Confirmed the downstream reach too — features.py:335 assigns it to _frame_valid, and parallel_workers.py:352 does labels[~identity_mask] = TrackLabels.Label.NONE, so it decides what is trainable. A migration mapping it to producer occupancy would have quietly admitted degenerate frames into hull and segmentation features.

Took option (2), with two adjustments:

Renamed to jabs.pose.slot_usable, alongjabs.pose.slot_occupied for occupancy. Copilot independently flagged that identity_present is the wrong name for tail slots — a tail slot never holds an identity, and those slots are exactly why we keep them. Since both masks are slot-indexed, slot_* is consistent. Your identity_usable is recorded as an open question (#8) if you prefer it; neither name says what the rule actually is, which is "enough confident non-tail keypoints to compute shape features".

The rule is declared in provenance as parameters: confidence_threshold, min_valid_keypoints, excluded_keypoints. The spec now carries your three-row table of frame states where the two masks disagree.

The cache claim is qualified rather than dropped, because I measured the derivation. point_valid[..., :-2].sum(-1) >= 3 over a 108,150-frame 3-identity mask: 3.2 ms, against 629 ms for the current np.vectorize + fromfunction, identical output — 199×. So identity_mask never needed caching for cost; the cache existed for the scatter. The bullet now says points and point_mask become free, and identity_mask is either declared by the producer or derived at load.

That leaves one thing genuinely open, recorded as #7: slot_usable is optional, so two consumers can still disagree about usable frames — the problem it exists to fix. Requiring producers to write it pushes a JABS consumer policy onto every producer, including foreign ones. Your call.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Several schema and encoding inconsistencies currently permit ambiguous or undecodable conforming files.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Proposes a self-describing, extensible HDF5 pose format to replace version-specific readers and caches.

Changes:

  • Defines manifest, component, encoding, provenance, and validation contracts.
  • Standardizes identity-aligned slots and explicit missing-data handling.
  • Documents conversion, storage, compatibility, and operational trade-offs.
File summaries
File Description
docs/development/adr/0002-unified-pose-file-format.md Adds ADR 0002 and the revision-1 pose format specification.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 12
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread docs/development/adr/0002-unified-pose-file-format.md Outdated
Comment thread docs/development/adr/0002-unified-pose-file-format.md Outdated
Comment thread docs/development/adr/0002-unified-pose-file-format.md Outdated
Comment thread docs/development/adr/0002-unified-pose-file-format.md Outdated
Comment thread docs/development/adr/0002-unified-pose-file-format.md Outdated
Comment thread docs/development/adr/0002-unified-pose-file-format.md Outdated
Comment thread docs/development/adr/0002-unified-pose-file-format.md Outdated
Comment thread docs/development/adr/0002-unified-pose-file-format.md Outdated
Comment thread docs/development/adr/0002-unified-pose-file-format.md Outdated
Comment thread docs/development/adr/0002-unified-pose-file-format.md Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The schema allows the NaN missing-value policy for incompatible non-floating-point dtypes.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (15)

docs/development/adr/0002-unified-pose-file-format.md:370

  • The sparse index component cannot be represented as described. .frame_index has a sample axis, so this conditional requires it to declare sparse, but the catalog only assigns sparse.index to .points and .counts. Either exempt index components from this rule or explicitly define and validate the index component's self-reference; otherwise the documented dynamic-object manifest fails schema validation.
        {
          "if": {
            "required": ["axes"],
            "properties": { "axes": { "contains": { "const": "sample" } } }
          },
          "then": { "required": ["sparse"] }

docs/development/adr/0002-unified-pose-file-format.md:906

  • Validation never ties an axis extent to the corresponding declared dimension. For example, a component with axes: ["frame", "slot"] and shape: [1, 1] can pass even when the manifest declares 108150 frames and 4 slots; video.frame_count can also disagree with dimensions.frame. Add an error check for these equalities so the two sources of shape metadata cannot silently diverge.
| declared `dtype` and `shape` match the dataset at `path` | error |
| `len(axes) == len(shape)` | error |

docs/development/adr/0002-unified-pose-file-format.md:358

  • A component with a keypoint axis is not required to provide skeleton, so a pose-points manifest can pass schema validation without defining what its keypoint indices mean. Make the reference conditional on the keypoint axis; the existing reference-resolution check will then also require the root skeleton definition.
      "allOf": [
        {

docs/development/adr/0002-unified-pose-file-format.md:604

  • Entries beyond contour_count are absent contours, but declaring none says every contour_length and external_flag entry is valid. This leaves padding semantically valid to generic readers, contrary to the explicit-absence rule. Use contour_count as the length policy for both contour-axis arrays.
| `jabs.segmentation.contour_length` | `/jabs/segmentation/contour_length` | frame, slot, contour | F×S×C | uint32 | `none` |
| `jabs.segmentation.external_flag` | `/jabs/segmentation/external_flag` | frame, slot, contour | F×S×C | bool | `none` |

docs/development/adr/0002-unified-pose-file-format.md:640

  • .counts declares how many objects are valid in each sample, but .points does not name it as its missing-value policy. Consequently, points in the remaining object slots are indistinguishable from real objects. Declare missing: {"policy": "length", "length": "<name>.counts"} for .points.
| `.frame_index` | sample | M | uint32 | frame | the frames on which a prediction was made, strictly increasing |
| `.points` | sample, object, point, coord | M×O×P×2 | float32 | pixel | `sparse.index` → `.frame_index` |
| `.counts` | sample | M | uint32 | unitless | valid object count per sample |

docs/development/adr/0002-unified-pose-file-format.md:532

  • The RLE representation does not define which value the first run represents. Identical run lengths can therefore decode to a mask or its complement in different readers. Specify the starting value and how a foreground first pixel is represented.
**RLE.** The payload at `path` holds concatenated run lengths in the given `order` (default
`column-major`, the COCO convention); `instance_offsets` is a `uint64` CSR index into it, in
row-major `(frame, slot)` order, with `len(instance_offsets) == F*S + 1`. Mask dimensions come from
`video.width` / `video.height`, which must therefore be non-null for a file using this encoding.

docs/development/adr/0002-unified-pose-file-format.md:349

  • szip cannot be faithfully described by this compression_opts type. HDF5/h5py represents SZIP options as a pair such as ("nn", 8), which serializes to a JSON array, whereas the schema only accepts an integer or null. Add the SZIP option-array shape (or remove szip) so declared layout can match the dataset.
        "storage": { "enum": ["contiguous", "chunked"] },
        "chunks": { "type": "array", "items": { "type": "integer", "minimum": 1 } },
        "compression": { "enum": ["none", "gzip", "lzf", "szip"] },
        "compression_opts": { "type": ["integer", "null"] }

docs/development/adr/0002-unified-pose-file-format.md:355

  • The schema does not require provenance, so a component can validate without any producer record despite the decision that provenance is per-component. Make the reference mandatory; the existing resolution check can then enforce the promised traceability for every component.
    "component": {
      "type": "object",
      "required": ["id", "path", "axes", "dtype", "shape", "encoding", "missing"],

docs/development/adr/0002-unified-pose-file-format.md:587

  • This frame/slot array has no valid instance in unoccupied slots, but the catalog does not define a missing policy for those embeddings. Declare mask → jabs.pose.slot_occupied, as bbox and tracklet_id do, so padding cannot be interpreted as an embedding.
| `jabs.identity.embeddings` | `/jabs/identity/embeddings` | frame, slot, embedding | F×S×E | float32 | per-instance identity embedding |

docs/development/adr/0002-unified-pose-file-format.md:905

  • These checks only compare each payload with its own manifest declaration; they never compare reserved jabs.* IDs with the component catalog. For example, jabs.pose.points can declare the wrong path, axes, dtype, or missing policy and still pass validation, leaving consumers to interpret the reserved ID inconsistently. Add an error check enforcing every defined JABS component's catalog contract.
| every component `path` exists in the file and holds that component's payload | error |
| declared `dtype` and `shape` match the dataset at `path` | error |

docs/development/adr/0002-unified-pose-file-format.md:558

  • This invariant is normative and JABS is stated to depend on it, but the validation rules never check it. A reused tracklet ID with a frame gap would therefore be reported as valid and can break consumers relying on contiguous intervals. Add an error check that every tracklet ID occupies exactly one contiguous frame interval.
`tracklet_id` values are gap-free intervals: a tracklet occupies a contiguous run of frames with no
breaks. This invariant is **normative** here rather than a footnote in a producer document, because
JABS depends on it.

docs/development/adr/0002-unified-pose-file-format.md:914

  • coord_order has exactly two positions (xy or yx), but neither the schema nor validation requires the coord axis to have length 2. A three-coordinate component can thus validate with an order that cannot describe its third value. Enforce the axis vocabulary's coordinate-pair contract.
| a component whose `axes` contain `coord` declares `units` and `coord_order` | error |

docs/development/adr/0002-unified-pose-file-format.md:912

  • The prose requires both offset arrays to be uint64, but these checks validate only their values and lengths. A float or signed offset dataset can pass the listed rules even though conforming readers are promised uint64. Include existence, rank, and dtype in the offset checks.
| ragged `group_offsets` is non-decreasing, starts at 0, ends at `shape[0]` of the payload, length `num_groups + 1` | error |
| ragged/RLE `instance_offsets` is non-decreasing, starts at 0, has length `frame*slot + 1`, and ends at `len(group_offsets) - 1` (ragged) or `shape[0]` of the payload (RLE) | error |

docs/development/adr/0002-unified-pose-file-format.md:331

  • The length policy names a component but never identifies which target axis it bounds or defines how the referenced axes must align. For example, contour_length is intended to bound point, while contour_count bounds contour; a generic reader cannot derive that rule from this schema. Add an explicit axis field, or normatively define axis-name alignment and the single unmatched axis that a length controls (and do the same for mask broadcasting).
        { "type": "object", "required": ["policy", "mask"], "additionalProperties": false,
          "properties": { "policy": { "const": "mask" },
                          "mask": { "$ref": "#/$defs/componentId" } } },
        { "type": "object", "required": ["policy", "length"], "additionalProperties": false,
          "properties": { "policy": { "const": "length" },
                          "length": { "$ref": "#/$defs/componentId" } } }

docs/development/adr/0002-unified-pose-file-format.md:614

  • When clipping ragged contours, external_flag has no frame/sample axis and no manifest link to the contour groups selected through instance_offsets. The generic clipping rule therefore cannot determine which flat flags to retain, so the flags become misaligned with the clipped payload. Declare their association with the ragged groups/offsets or encode them as part of the same grouped component.
Under the ragged encoding, `jabs.segmentation.contours` has `axes: ["point", "coord"]` and its
`path` holds the concatenated contour points; `group_offsets` is
`/jabs/segmentation/contour_offsets` and `instance_offsets` is
`/jabs/segmentation/instance_offsets`. `external_flag` becomes a flat `contour`-axis array.
`contour_count` and `contour_length` are then derivable from the offsets and must be omitted.
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread docs/development/adr/0002-unified-pose-file-format.md
bergsalex and others added 3 commits September 4, 2026 16:04
Proposes one self-describing pose file to replace pose_est_v2 through v8,
with a complete revision-1 specification: root identification, a JSON
manifest with its JSON Schema, the component catalogue, encodings,
provenance, attachments and the validation rules.

The organising idea is that a reader asks what a file contains, never how
old it is. A manifest declares each component's axes, dtype, units,
coordinate order and missing-value policy, so evolution is additive and
data JABS does not understand can be subset and validated by tools that
know nothing about its meaning.

Two decisions carry most of the benefit. Slot k means identity k, which
retires the scatter-and-flip currently reimplemented in pose_est_v4.py,
the cache writer and JABS-postprocess's clip_utils.py, and with it the
per-video cache file. And absence is always stated -- NaN or an explicit
mask or length, never a sentinel -- which removes the 0-is-both-padding-
and-a-valid-pixel ambiguity that makes every consumer re-derive validity.

Measured rather than assumed: instance_embedding is 5.19 MB per mouse-hour
that nothing reads, while identity_embeds and instance_id_center look
equally vestigial from inside JABS but are read by JABS-postprocess for
cross-video linking. seg_data is 99.2% padding in the v8 case, though
ragged encoding would not shrink the file -- gzip already removes the
padding -- so dense stays the baseline.

The schema in this document was checked: it is valid draft-2020-12 and the
example manifest validates against it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…that enforces its own contract

The review found that identity_mask is not an occupancy flag. Verified:
pose_est_v4.py:159-172 computes it as sum(point_mask[..., :-2]) >= 3 --
three keypoints above MINIMUM_CONFIDENCE excluding both tail points,
because the convex hull needs three. It becomes _frame_valid in
features.py:335 and forces labels to NONE where false
(parallel_workers.py:352), so it decides what is trainable. Mapping it to
a producer occupancy flag would have admitted degenerate frames into hull
and segmentation features.

So the two masks are now distinct: slot_occupied (an instance is here) and
slot_usable (enough confident non-tail keypoints), the latter declaring
its threshold, minimum and excluded keypoints in provenance -- the same
argument this ADR already makes about MINIMUM_CONFIDENCE being re-derived
by every consumer.

Renamed from identity_present, because a tail slot never holds an identity
and the mask exists precisely for those slots.

The cache-elimination claim is corrected rather than dropped: points and
point_mask become free, and identity_mask is cheap to derive --
point_valid[..., :-2].sum(-1) >= 3 measures 3.2 ms against 629 ms for the
current np.vectorize implementation over 108,150 frames, identical output.
The cache existed for the scatter.

Schema fixes, each with a negative test:
- video is now required (RLE needs its dimensions; the file is per-video)
- missing is required; units and coord_order required on coord components
- ragged requires group_offsets, without which it cannot be decoded
- a sample axis requires sparse, and vice versa
- payload always lives at the component path; encodings name only indexes
- provenance gets its own JSON Schema, with convert entries required to
  carry source and synthesized

Also: the two-level CSR decode formula confused group and payload index
spaces; sparse components now use a distinct sample axis so a clip tool
cannot slice a frame-number index as per-frame values; the single-byte-range
claim is corrected -- only contiguous storage gives it, chunking gives
bounded amplification -- and components can now declare their layout.

American English per repo convention (behavior 153/0, center 16/0).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commit's schema only required `sparse` when a `sample` axis
was present; the reverse -- `sparse` implies a `sample` axis -- lived only
in the validator table, though a review reply claimed the schema enforced
both. Adds the reverse conditional so the claim holds.

Verified: a sample axis without sparse is rejected, sparse without a
sample axis is rejected, and the two together are accepted.

Also names how a converter derives slot_occupied from v4-v8 (any keypoint
confidence > 0, matching instance_count's documented meaning), which the
v2 row already stated and the v4-v8 row did not.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@bergsalex
bergsalex force-pushed the docs/adr-0002-pose-format branch from d354c72 to fa77184 Compare September 4, 2026 20:04
@bergsalex

Copy link
Copy Markdown
Collaborator Author

Reviewed from the JABS Hub side, since Requirement 2 (partial/range reads over cloud object storage, for the in-browser pose overlay) is our consumer. Two contributions and one flag.

Open question 1 — do the recording devices drop frames?

Hub can answer this, and I think it is the only place that can, because it already keeps intent and reality separable on purpose.

  • Intent is recorded and non-null: recording_sessions.target_fps integer NOT NULL CHECK (target_fps >= 1) (0002_recording_sessions.up.sql:5).
  • Reality is whatever a pose artifact reports as its frame count.
  • Crucially, 0024_backfill_video_fps.up.sql refused to manufacture the comparison, and said why: "num_frames and duration_seconds are NOT backfilled. Deriving a frame count from duration x target_fps would manufacture a number that consumers compare against the real one on a pose artifact; a disagreement would then be an artifact of this migration rather than a fact about the data."

So target_fps × duration_seconds vs the artifact's real frame count is a valid drop detector today, and nobody has run it. The honest caveat: 0022_artifact_defaults.up.sql records that "the deployed system is believed to hold no pose files", so there is currently nothing to compare against — this becomes answerable as soon as the pipeline produces real artifacts, not before.

One correlated signal if you want a mechanism rather than just a count: Hub stores client_system_info.disk_pressure (0006), a health signal from the capture hosts that exists as a scheduling brake. I have not checked whether pressure episodes coincide with short recordings — flagging it as the obvious place to look, not as a finding.

If it turns out drops are real, I read that as strengthening the case for making jabs.time.timestamps required on natively produced files, exactly as the question suggests.

A flag: cataloguing a jabs.pose-file is a contract change, not a free one

Hub's catalogue can record a legacy pose version and has nowhere to record a pose format. video_artifacts.pose_version is a smallint, constrained by CHECK (kind = 'POSE' OR pose_version IS NULL). A jabs.pose-file has no version to put there — schema_revision exists but the spec rightly forbids branching on it — so it lands NULL, which is indistinguishable from "legacy pose of unknown version". A second, smaller consequence: name_suffix documents a fallback to _pose_est_v{pose_version}.h5 when the suffix is absent (0018_video_artifacts.up.sql:56), which produces a wrong filename for a file that is not versioned that way.

The natural place to fix it is the artifacts.json manifest the pipeline writes, since ADR-0012 D3 makes the producer supply artifact metadata. That is a versioned three-repo contract (Hub, pipeline, JABS-behavior-classifier) at ManifestSchemaVersion = 1, and Hub hard-fails on a schema_version newer than it understands. So a jabs_format discriminator has to be added as optional-within-major or older Hubs reject the manifest outright. Not a blocker for this ADR — recording it here so the format's rollout does not discover it late. I will raise the Hub-side ticket.

Smaller notes

  • Design goal 1 ("Inference output only, read-only once written. Labels, features, predictions and classifiers stay where they are") is the right boundary for us and I have adopted it: human corrections in the Hub go into a relational label store, never back into the pose file.
  • The chunking section is the most valuable part of this ADR from where I sit. "That works today only because points and confidence happen to be stored contiguous and uncompressed. The policy turns that accident into a stated intent" is exactly right — our overlay depends on that accident today. Spelling out that a chunked frame window is not one byte range, with amplification bounded at ceil(w/c)+1, and putting layout.chunks in the manifest so a reader can tell before reading, answers a question we currently have open on our side.
  • The Negative bullet "per-identity whole-video reads become strided rather than contiguous ... wasteful over a network" is a Hub-shaped cost, and I think it is acceptable: our review surface reads a small frame window across all slots, which stays frame-major and contiguous. It would bite a long single-identity range, which we do not plan to do over the network.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Critical schema, axis-consistency, and RLE specification issues remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

docs/development/adr/0002-unified-pose-file-format.md:325

  • This schema permits { "policy": "nan" } for integer, boolean, and string components, contradicting the rule that only floats use NaN and all other missing values use a mask or length. Such a manifest passes schema validation but cannot represent its declared missing value. Add a component-level dtype/policy constraint (and validator check) that limits nan to float32/float64.
        { "type": "object", "required": ["policy"], "additionalProperties": false,
          "properties": { "policy": { "enum": ["none", "nan"] } } },

docs/development/adr/0002-unified-pose-file-format.md:140

  • This design goal requires keypoint-scale data to be chunked, while the normative storage policy below recommends contiguous storage whenever frame count is known. Align the goal with that policy so producers are not given contradictory requirements.
13. **Keypoint-scale data is uncompressed and chunked along frames**; segmentation-scale data is
    chunked and compressed.

docs/development/adr/0002-unified-pose-file-format.md:1049

  • The stated measurement compares an uncompressed ragged estimate (~87 MB) against a gzip-compressed dense dataset (16.5 MB), even though the selected policy also compresses ragged segmentation; the measurement note also says offsets were excluded. This does not establish that ragged storage would be larger or would not reduce stored size. Measure compressed ragged payload plus offsets, or qualify the conclusion.
**Ragged segmentation as the baseline.** Measurement showed it does not reduce stored size — gzip
already compresses runs of `-1` to almost nothing, so ragged's ~87 MB of real int32 points for a
mouse-hour is *larger* than the 16.5 MB the padded array occupies. Its real gains are a 13× smaller
uncompressed footprint, per-frame random access, and the removal of shape-baked caps. Permitted,
  • Files reviewed: 1/1 changed files
  • Comments generated: 8
  • Review effort level: Balanced

Comment thread docs/development/adr/0002-unified-pose-file-format.md
Comment thread docs/development/adr/0002-unified-pose-file-format.md Outdated
Comment thread docs/development/adr/0002-unified-pose-file-format.md
Comment thread docs/development/adr/0002-unified-pose-file-format.md
Comment thread docs/development/adr/0002-unified-pose-file-format.md
Comment thread docs/development/adr/0002-unified-pose-file-format.md
Comment thread docs/development/adr/0002-unified-pose-file-format.md
Comment thread docs/development/adr/0002-unified-pose-file-format.md Outdated
… open

Eight review findings, all of them places where a conforming file could
still be ambiguous or undecodable.

Schema conditionals, each with a negative test against the worked example:

- `missing.policy: "nan"` is now restricted to float32/float64. NaN is
  reserved for floats by design goal 11, and integer, boolean and string
  payloads cannot represent it, so the policy was declarable where it has
  no meaning.
- A component with a `keypoint` axis must declare a `skeleton`. Design
  goal 10 says the skeleton is required, but only `points` was carrying
  one; `confidence` and `point_valid` inherited it by assumption. Every
  keypoint component now names it, so a reader holding one component's
  entry never has to consult a sibling's to learn what its keypoint axis
  means.
- An RLE payload must be an unsigned integer. A run length is a count, and
  the general component schema would otherwise admit floats or strings.

RLE was under-specified in two further ways that would have had two
conforming readers decode the same file differently: each instance's runs
now begin with background (so a foreground-first mask starts with a
zero-length run, rather than leaving alternation to guesswork), and each
instance's runs must sum to exactly width*height, which no shape check can
catch because the payload is a flat buffer.

The catalog states conventions it had been leaving to producers:
segmentation contours carry pixel/xy explicitly, and dynamic objects now
declare `.points` as `length -> .counts` with `coord_order: xy`.

The validation table gains the ten rows the implementation already
enforces, or now does -- axis lengths against dimensions, dimensions
against video.frame_count, skeleton edge bounds, mask axis alignment,
offset integrity, and the RLE coverage rule.

**The self-reference question is resolved rather than left open.** The
dynamic-objects catalog now shows `.frame_index` naming itself as its own
sparse index, with the reason: it is what makes the generic clipping rule
apply to the index too, instead of needing a special case for the one
component that describes the others. This matches the shipped schema and
the implementation. @gbeane, this was the item I had left for you -- say
so if you would rather relax the bidirectional rule instead.

One finding is answered rather than implemented: per-component provenance
stays optional. Design goal 15 says provenance is *per-component*, which
is about where records attach, not that every component must carry one,
and requiring it in the schema would contradict design goal 16 -- a
segmentation-only file from a third party would become invalid for
omitting a field only JABS cares about. It is a warning-level check
instead, with the reasoning recorded in the conformance section.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Critical clipping and attachment-validation gaps, plus unresolved schema inconsistencies, must be addressed.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (12)

Previously missed (11) — in code that hasn't changed since the last review.

docs/development/adr/0002-unified-pose-file-format.md:241

  • string does not define an interoperable HDF5 representation: fixed- and variable-length strings and ASCII/UTF-8 character sets produce different h5py values, yet all currently map to this one manifest dtype. Define the accepted representation (preferably variable-length UTF-8) and how the validator matches it, particularly for jabs.identity.external_ids.
    docs/development/adr/0002-unified-pose-file-format.md:349
  • The schema advertises szip but cannot represent its HDF5 options: SZIP uses a two-item coding/pixels-per-block tuple, while compression_opts permits only an integer or null. Either remove SZIP from the supported enum or add the JSON-array form (with filter-specific validation), otherwise an accurately described SZIP dataset cannot satisfy this schema.
    docs/development/adr/0002-unified-pose-file-format.md:420
  • units is optional for non-coordinate components, but the specification never defines what omission means. Consequently, an unknown unit and a genuinely unitless value are indistinguishable for foreign components. Preserve the intended optional field by stating a normative unitless default, or require producers to declare units explicitly.
    docs/development/adr/0002-unified-pose-file-format.md:607
  • The required confidence threshold is not enforceable as specified: component provenance is optional, and the provenance schema leaves parameters unconstrained. A valid jabs.pose.point_valid component can therefore omit the threshold that consumers need to interpret it. Add component-specific validation requiring a referenced record with a numeric parameters.confidence_threshold whenever this component is present.
    docs/development/adr/0002-unified-pose-file-format.md:611
  • The catalog declares contiguous tracklet_id intervals normative because JABS relies on them, but the validation contract never checks that invariant. Files with one tracklet ID reappearing after a gap would validate and then violate a stated reader assumption; add an error-level contiguity check when this component is present.
    docs/development/adr/0002-unified-pose-file-format.md:631
  • The three parameters said to define slot_usable are not required by either schema or the validation table, and provenance itself is only warning-level. Thus a valid file may contain this mask without enough information to reproduce or evaluate its quality rule. Require component-specific provenance with typed confidence_threshold, min_valid_keypoints, and excluded_keypoints whenever jabs.pose.slot_usable is present.
    docs/development/adr/0002-unified-pose-file-format.md:642
  • These catalog entries omit missing, although every component manifest is required to declare it. This is especially ambiguous for frame/slot embeddings because unoccupied slots need an explicit absence policy. Add a missing-policy column and define whether embeddings use NaN or jabs.pose.slot_occupied, plus the policies for centers and external IDs.
    docs/development/adr/0002-unified-pose-file-format.md:657
  • These arrays also contain padded contour slots, so missing: none incorrectly declares every padded length/flag meaningful and contradicts the statement below that contour_count determines validity. Make both components length-masked by contour_count; this also lets generic readers apply the hierarchy without special-casing segmentation.
    docs/development/adr/0002-unified-pose-file-format.md:683
  • The component schema requires missing, but the static-object catalog does not specify a policy, so producers cannot construct a complete manifest from this definition. If a present static-object component always contains valid coordinates, declare missing: none here.
    docs/development/adr/0002-unified-pose-file-format.md:723
  • jabs.time.timestamps lacks the mandatory missing-value declaration, leaving its catalog entry insufficient to produce a schema-valid component. Since unknown timestamps can be represented by omitting this optional component, specify none for a present timestamp array.
    docs/development/adr/0002-unified-pose-file-format.md:971
  • The root and manifest each carry schema_revision, but no check requires them to agree (or verifies the root attribute's specified int32 value). A file with root revision 1 and manifest revision 2 can pass this contract while reporting conflicting diagnostics. Validate the root type/value and equality with the manifest field.

docs/development/adr/0002-unified-pose-file-format.md:672

  • Flattening external_flag removes its frame/slot axes, but the manifest contains no reference tying its local contour axis to this component's group_offsets. After clipping or re-encoding contours, a generic tool cannot determine which flags to retain or reorder. Add an explicit shared-group/reference relationship so this sidecar remains aligned through transformations.
Under the ragged encoding, `jabs.segmentation.contours` has `axes: ["point", "coord"]` and its
`path` holds the concatenated contour points; `group_offsets` is
`/jabs/segmentation/contour_offsets` and `instance_offsets` is
`/jabs/segmentation/instance_offsets`. `external_flag` becomes a flat `contour`-axis array.
`contour_count` and `contour_length` are then derivable from the offsets and must be omitted.
  • Files reviewed: 1/1 changed files
  • Comments generated: 6
  • Review effort level: Balanced

Comment on lines +668 to +672
Under the ragged encoding, `jabs.segmentation.contours` has `axes: ["point", "coord"]` and its
`path` holds the concatenated contour points; `group_offsets` is
`/jabs/segmentation/contour_offsets` and `instance_offsets` is
`/jabs/segmentation/instance_offsets`. `external_flag` becomes a flat `contour`-axis array.
`contour_count` and `contour_length` are then derivable from the offsets and must be omitted.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Correct, and this is the sharpest of the six — the extensibility claim ("declared axes let generic tooling subset unknown components") simply does not hold for a non-dense component, and the document asserted the claim without qualifying it.

Fixed in 48647de with a new Clipping section that states the contract rather than leaving it to be discovered:

  • A tool that does not implement a component's encoding must refuse to clip the file, naming the component and the encoding. It must not drop the component, and it must not copy it through with stale offsets — which is exactly the case you describe, where retained offsets then describe more frames than dimensions.frame claims.
  • A tool that does implement it follows a stated procedure: instance_offsets is row-major over (frame, slot), so frames [a, b) are rows [a*S, b*S]; take that range, rebase to zero, slice group_offsets over the groups those rows span and rebase likewise, then slice the payload over the points those groups span.
  • Either way a clip history entry records the frame offset.

For the sidecar association I reused the existing mechanism rather than adding a field: external_flag under the ragged encoding declares the same encoding, naming the same two offset datasets. A tool holding the manifest can then see that the two components are indexed by one pair of offsets, instead of having to know that one happens to be per-contour, and the clipping procedure above covers it by the same group range.

Whether jabs-io should implement ragged clipping now, or keep refusing until a producer actually writes a ragged file, is recorded as an open question — it is a scheduling call, not a specification gap any more.

| `video.width` / `video.height` non-null | warning |
| every component declares a `provenance` record | warning |
| declared `layout` matches the dataset's actual HDF5 storage and filters | warning |
| an `/attachments` member has no manifest entry | warning |

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Correct — only the reverse direction was checked. Fixed in 48647de (spec) and 20e2444 (code): existence, namespace and uniqueness are all error-level.

The asymmetry is deliberate and now stated: an undeclared payload under /attachments/ is a warning, because it costs nothing and loses nothing, while a declaration with no payload is an error, because a copy tool is required to preserve something that is not there. Three tests, one per rule.

Comment on lines +394 to +398
"then": {
"properties": {
"dtype": { "enum": ["uint8", "uint16", "uint32", "uint64"] }
}
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Correct. Fixed in 48647de: the conditional now requires axes to be exactly ["run"] alongside the unsigned dtype, so a [N, 2] payload can no longer coexist with offsets that terminate at shape[0]. Verified with a probe against the ADR's own example — a 2-D RLE payload is rejected, a flat one accepted. The run axis was already in the axis vocabulary and simply unused.

| every component `path` exists in the file and holds that component's payload | error |
| declared `dtype` and `shape` match the dataset at `path` | error |
| `len(axes) == len(shape)` | error |
| a `mask` or `length` reference resolves to an existing component with a compatible shape | error |

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Correct, and the phrase is worth keeping: alignment is not interpretability. A boolean mask says present or absent and a length says how many, so neither is applicable with the wrong dtype however well its shape lines up.

Fixed in 48647de (validation rows) and 20e2444 (both PoseFile construction and validate()): a mask reference must be boolean; a length reference must be a non-negative integer no larger than the axis it bounds. Five tests, including the accepting case — the dynamic-object length → .counts pattern the ADR now specifies.

| every keypoint component's `keypoint` axis length equals its skeleton's `body_parts` length | error |
| `dimensions.identity <= dimensions.slot` | error |
| `dimensions.frame == video.frame_count` — the same fact, stated twice, must agree | error |
| every axis that names a dimension (`frame`, `slot`, `identity`) has that dimension's length | error |

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Correct and embarrassingly simple — coord is normatively a pair and nothing checked it. Fixed in 48647de and 20e2444: exactly 2, refused at construction and reported as coord_axis_length for a file another producer wrote.

| a component with a `keypoint` axis declares a `skeleton` | error |
| every skeleton edge index is `< len(body_parts)` | error |
| a `mask`/`length` reference's axes are the target's leading axes, not merely the same lengths | error |
| ragged `group_offsets` / `instance_offsets`, and RLE `instance_offsets`: present, one-dimensional, non-decreasing, starting at 0, ending at the correct terminal value, with `frame*slot+1` instance entries | error |

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Correct. The prose mandated uint64 and the validation contract checked only dimensions and values.

Fixed in 48647de (row) and 20e2444 (code): offset datasets must be unsigned integers. Worth noting the old behaviour was doubly wrong — a float offsets array was reported as "missing or not a one-dimensional dataset", which is a misleading message for a dataset that is present and one-dimensional. The message now names the real problem. Parametrized tests cover float64 and int64 rejection and uint64 acceptance.

…ll left open

Six findings against the round-2 commit, all of them cases where a
conforming file could still be uninterpretable.

**The generic clipping rule did not cover non-dense components, and the
document did not say so.** A ragged `contours` component has neither a
`frame` nor a `sample` axis, because frame ownership lives in
`instance_offsets` — so a tool slicing only what it recognised would leave
stale offsets describing more frames than `dimensions.frame` claims. A new
Clipping section states the contract: a tool that does not implement an
encoding must refuse and name it, never drop the component and never copy
it through with its offsets untouched; a tool that does implement it
follows the stated procedure over `(frame, slot)` row-major
`instance_offsets`. Whether to implement ragged clipping before a producer
writes a ragged file is recorded as an open question.

**Per-contour sidecars now have a machine-readable association.**
`external_flag` under the ragged encoding declares the same encoding,
naming the same two offset datasets, so a tool can see from the manifest
that the two components share one pair of offsets rather than having to
know that one happens to be per-contour. That reuses the existing
mechanism instead of adding a sidecar field.

**An RLE payload must be one-dimensional with a single `run` axis.** The
conditional constrained only dtype, so a `[N, 2]` payload with arbitrary
axes stayed schema-valid even though the offsets terminate at `shape[0]`
and the prose defines a flat sequence of scalar runs.

Four validation-table rows for constraints the prose already implied:
a `coord` axis has length exactly 2 (`coord_order` describes two values and
nothing else); offset datasets are unsigned integers, since a float cannot
index an array; a `mask` reference is boolean and a `length` reference a
non-negative integer bounded by the axis it describes, because shape
compatibility alone does not make either interpretable; and a declared
attachment must exist, live under `/attachments/`, and be declared once.

That last one was asymmetric in a way worth naming: an undeclared payload
under `/attachments/` is only a warning, because it costs nothing and
loses nothing, while a declaration with no payload is an error, because a
copy tool is required to preserve something that is not there.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants