Skip to content

Foundation: Box evaluation report and gold Box label format (#311) - #331

Merged
CompleteDotTech merged 3 commits into
mainfrom
foundation/box-evaluation
Oct 21, 2026
Merged

CompleteDotTech merged 3 commits into
mainfrom
foundation/box-evaluation

Conversation

@CompleteDotTech

@CompleteDotTech CompleteDotTech commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Closes #311

What this adds

  • vision.gold_box(label): decodes and validates the gold Box label {"present": true, "box": {top, bottom, left, right}} / {"present": false} (strict keys, boolean present, no box on an absent label, edges in [0, 1], positive size, and a gold area that does not underflow to zero, so box_iou can never hit Foundation: enforce the Images API's 280 px minimum, hash image bytes in replay keys, bound tiles (#306) #321's zero-union error against a gold box).
  • vision.localization_report(gold, outcomes, *, iou_threshold=0.5) and LocalizationReport: takes ("located", box) / ("absent", None) / ("review", None) per example; a present table and an absent table count every example exactly once; mean IoU over located-present and over all present (missed and review count 0), hit rate at the threshold, false positives, correct absents; str(report) and report.denominators name every denominator. A box returned for an absent target is a false positive, never an IoU; a review is never an absent prediction.
  • Fixture validator: a top-level label object with a present key declares Box labels; each must pass gold_box, and a non-Box label among them is an error. schema.json description points to the format.
  • localization_metrics kept; its docstring and docs/vision.md point to the report.
  • Docs: docs/fixtures.md ("Box labels"), docs/vision.md (gold labels, the report, the worked example), docs/evaluation.md (Box localization table of denominators).

Design note for the reviewers: a Box label nested under another key (rank 98 mixed types, {"target": {"present": ..., "box": ...}, "rubric": 3}) is not seen by the validator; such recipes call gold_box(label["target"]) themselves. A tile choice is not stored beside a gold box: the box is stored once, full-image, and Python derives tiles from it (docs/vision.md, "Tiled and hierarchical recipes"; docs/fixtures.md, "Box labels"). "Declared" is by the reserved top-level present key because no per-recipe declaration mechanism exists. Raise it if you prefer an explicit marker.

Recipe checklist

Foundation work; section skipped.

How it was verified

Merged head 23182da (signed merge of origin/main c316712 into 865dfcc), Linux, fresh Python 3.10 venv:

  • ruff check .: All checks passed. ruff format --check .: 397 files already formatted.
  • pytest: 5409 passed, 49 skipped, 50 deselected, 31 xfailed. (865dfcc also passed on Python 3.14: 5408 passed.)
  • python tools/render_catalog.py --check: exit 0. python tools/check_hygiene.py: passed, 606 files. python tools/check_recipe_assets.py: passed. python tools/notebook_ci.py fixtures: 51 recipe folders, 0 failed.
  • No notebook touches vision.localization_*, so no notebook execution is affected.
  • New tests (tests/test_vision.py, tests/test_fixtures.py) use hand-computed values on dyadic rectangles: mixed, all-absent, all-review, a reviewed present target changing the denominator, a box returned for an absent target, a present target called absent, a degenerate (underflowing) gold box, threshold inclusivity, malformed inputs; the vision.md worked example is executed and its printed text compared. Two docs-structure tests pin the Box-labels examples and the tiled-recipe statements.

…r check (#311)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@CompleteDotTech

Copy link
Copy Markdown
Contributor Author

Opus reviewer (independent review 2 of 2): Claude Opus 5.5 (claude-opus-5-5).

Reviewed head 34249bf661d919291532ed84ebf1e9cf8057c808 against base e998eb59d78ee06b32c8346c9932a09e6605a45e. I did not read the other review before writing this one.

The code is correct, and the mutation tests show its rules are enforced. gold_box, localization_report and the validator hook are what #311 asks for, and I would accept them unchanged. My two blocking findings are both in the docs. Ranks 83 and 98 will copy the nested-label and tiled-search guidance exactly as written, and as written it teaches the wrong pattern.

Blocking

  1. docs/fixtures.md:129-132: the nested-label example is contradictory, and rank 83 will copy it. {"tile": "b3", "target": {"present": false}} names a gold tile for a target that is not in the image. For an absent target, the correct tile choice is "no match", not b3. The example also stores the tile choice as a separate label next to the box, which causes two problems. First, nothing checks that the stored tile actually contains the stored box. Second, with generate_tiles(..., overlap=...) a target near a seam lies in more than one tile, so a single string cannot hold the gold. My sketch put a 1200x900 image into 2x2 tiles of 640x480 with 80 px overlap, and a target on the vertical seam was present in both a1 and a2. Change: use an example that cannot contradict itself (for instance {"target": {"present": true, "box": {...}}, "rubric": 3} for rank 98), and add one sentence for rank 83: store the gold once, as a full-image Box label, and have Python derive which tiles count as correct from that box and the tile bounds. Do not store a tile label.

  2. docs/vision.md, "The localization report" (around lines 172-245), does not say what one example is when a recipe tiles the image. Rank 83 is the case the task asks about. A target can be present in the full image and absent from most tiles. The report handles this correctly in both of the ways I ran it:

    • Per image: gold is the full-image label and the outcome is the final rectangle after tile_to_image. A stage-1 "no match" on present gold becomes ("absent", None), a miss. An uncertain tile choice becomes ("review", None).
    • Per tile: gold is derived for each tile. The empty tiles land only in the absent table, so they never inflate mean IoU or hit rate.

    The risk is the headline number. Nothing in the docs stops a rank 83 builder from quoting the per-tile report, where n_total counts tiles and correct_absent_rate mostly measures empty tiles, as the recipe's result. The per-tile view also needs a recipe-level rule for a target that a tile edge cuts. The format has only present or absent, so whether a target that is half visible in a tile is "present" changes the per-tile denominators. Change: add a short paragraph (or a row in the docs/evaluation.md table) saying:

    • in a tiled or hierarchical recipe the headline report is per image, against the full-image gold, computed after tile_to_image;
    • a per-tile report is a stage diagnostic, labelled as counting tiles, that states its rule for partly visible targets;
    • stage-1 "no match" maps to absent and an uncertain choice maps to review.

Suggestions (not blocking)

  • No helper maps gold into a tile. The rank 83 sketch had to write its own image-to-tile clip: it intersects the gold box with the tile bounds, applies a minimum visible fraction, and re-normalizes. That is the inverse of tile_to_image, and edge-snapping mistakes are likely if each recipe writes it. A follow-up image_to_tile(box, tile_bounds, width, height) with tests would fit next to tile_to_image. It does not belong in this PR.

  • Ambiguous hard cases. docs/multimodal-track.md asks for ambiguous examples, and rank 77's natural hard case is two identical "Delete" buttons. The format has no gold that says "review is the right answer", so a correct review on such an example counts as 0 in mean_iou_present and as a miss in the hit rate. I think that strictness is defensible, but say so in "Box labels": the question criteria must pick out one rectangle, the label is that rectangle, and a review on it is the honest cost of abstaining. Point to evaluate_outcomes on the presence question for the abstention view.

  • The top-level present reservation (the design note's question). I accept the implicit declaration because every failure it causes is loud, never silent:

    • A Noul-only recipe whose label is naturally {"present": true, "severity": "high"} (ranks 61-68 or 78 could do this) will be rejected as a "malformed Box label". The author renames the key.
    • A Box label with an extra key (for example rank 77 wanting "action": "click" beside the box) is rejected the same way.
    • The silent direction, a Box recipe that leaves out present, is caught later, because localization_report decodes every gold label with gold_box and raises with the example index.

    An explicit marker can wait until a per-recipe declaration mechanism exists. One improvement: when a label has present but fails, have the error say that present is reserved for Box labels, so the Noul author knows to rename.

  • Is the nested case (83, 98) safe without validator support? Yes for every split the recipe reports. localization_report calls gold_box on each gold label and raises example N: ..., and offline CI runs the notebook. My sketch confirmed that a nested, inverted box raises at report time. The gap is a split that never goes through the report (for example train in rank 98), where a malformed nested label passes unnoticed. docs/fixtures.md already tells the recipe to call gold_box(label[...]) in its own test. Say explicitly that the test must cover every split.

  • Review count. LocalizationReport has no single total for reviews across present and absent targets. A n_review property and one printed line would save every README from adding the two cells by hand.

How each recipe would use the interface

I wrote a sketch for this (run offline, with scripted answers, using this branch's jev_cookbook.vision):

  • 77 (Box + Noul, reference recipe).

    • Labels: top-level {"present": ..., "box": ...}.
    • Presence gold: gold_box(l) is not None.
    • Python gate: the localization_outcome from docs/vision.md. An absent or review result is never clicked, and a located result previews a click at the centre of the rectangle.
    • Report: localization_report(labels, outcomes).

    It fits cleanly.

  • 83 (Choice + Box, tiled). It fits at the per-image level, with full-image gold and the final mapped rectangle. The per-tile level needs derived gold plus the clipping rule (blocking 2). The stored tile label should be removed (blocking 1).

  • 98 (all four types). Box gold is nested per task type. Each compared method gets its own localization_report against the same gold. The ious tuple has one entry per example, so methods can be compared example by example. It fits.

Checks run (fresh venv, pip install -e ".[dev,ml]", Windows, no TYPESAFE_API_KEY)

  • ruff check .: all checks passed. ruff format --check .: 397 files already formatted.
  • pytest: 5391 passed, 58 skipped, 31 xfailed, 3 failed. Two of the failures are the known Windows-only signal.SIGKILL failures in tests/test_notebook_ci_sandbox.py (Foundation: two notebook-CI sandbox tests use signal.SIGKILL and fail on Windows #302). The third, tests/test_new_recipe.py::test_a_kernel_start_failure_is_retried_once_and_then_the_notebook_runs[Kernel died before replying to kernel_info], passed when rerun on its own (2 passed). It looks like a load-timing flake: it is in code this PR does not touch, and it may have overlapped with my mutation runs.
  • tests/test_vision.py + tests/test_fixtures.py (rerun on their own): 475 passed, 2 skipped.
  • python tools/render_catalog.py --check: rc 0. python tools/check_hygiene.py: passed, 606 files. python -m jev_cookbook.fixtures validate --all: every recipe valid, rc 0.
  • Scope: git diff --raw -M origin/main...HEAD shows 8 files, all M with mode 100644 -> 100644. There is no symlink, no submodule and no mode change, and the files are those Foundation: Box evaluation can drop reviewed and absent cases from the IoU denominator, and there is no label format for an absent target #311 lists, plus schema.json, which changes only a description.
  • Notebooks: no recipe imports localization_* or gold_box (grep over recipes/), so no notebook's behaviour can change. CI notebook jobs: every check on the head passes (gh pr checks 331).
  • Mutation tests (each change applied alone, then reverted; tests run were test_vision.py and test_fixtures.py). The tests caught all 8:
    • a review on present gold counted as absent: 5 tests failed;
    • the validator's _check_box_labels call removed: 13 failed;
    • the hit comparison changed from >= to >: 5 failed;
    • a box allowed on an absent label: 6 failed;
    • the all-present mean divided by the located count: 4 failed;
    • a non-Box label allowed among Box labels: 1 failed;
    • the area-underflow check removed: 3 failed;
    • a box on absent gold counted as a correct absent: 5 failed.
  • Hand checks: I checked the worked example's numbers myself (mean IoU over located targets 0.75 = (1 + 0.5) / 2, over all present targets 0.375 = 1.5 / 4, hit rate 2/4). A test runs the docs code and compares its printed text exactly.

Acceptance criteria (#311)

  • Gold Box label format in docs/fixtures.md, vision.gold_box with tests, and the validator rejecting malformed Box labels when declared: met. The declaration is implicit, through the top-level present key. The nested-label example must be fixed (blocking 1).
  • localization_report signature and outcome pairs; n_total; present and absent tables counting each example once; both mean IoUs with named denominators; hit rate; false positives; correct absents: met.
  • A printable form that names every denominator; a test showing that a reviewed present target changes the denominator; hand-computed values: met.
  • localization_metrics kept, with its docs pointing to the report: met.
  • docs/vision.md and docs/evaluation.md updated, and the worked example matches the test: met for the worked example. The guidance for tiled recipes is missing (blocking 2).

OPUS-REVIEW: CHANGES-NEEDED 34249bf

@CompleteDotTech

Copy link
Copy Markdown
Contributor Author

Opus reviewer (independent review 1 of 2): Claude Opus 5.5 (claude-opus-5-5)

Reviewed head 34249bf661d919291532ed84ebf1e9cf8057c808 against base e998eb59d78ee06b32c8346c9932a09e6605a45e. I wrote this without reading the other review.

Blocking findings

None.

Check against docs/multimodal-track.md, "What an evaluation must show" (Box row)

  • IoU only on present targets with a returned box after a presence check. Met. localization_report (src/jev_cookbook/vision.py, the loop after for index, (label, outcome)) appends an IoU only when the gold label is present and the outcome is located. located is what the documented localization_outcome policy (docs/vision.md:143) returns after the Noul presence decision and the confidence threshold. The report cannot check that the caller actually ran a presence check, and no function taking only (kind, box) could. Keeping the policy in recipe Python and the scoring here is the right split.
  • Absent-target and review outcomes kept separate, never silently dropped. Met. Each example falls into exactly one cell of a 2 x 3 table. A review is never counted as a miss or as a correct absent (the all-review test asserts correct_absents == present_missed == 0). str(report) prints the review count on both the present line and the absent line.
  • Every rate states its denominator. Met. Every mean and rate in str(report) prints "(denominator N: what)", and rates also print k/N. A zero denominator prints n/a and the value is NaN, never 0. report.denominators gives them by name.
  • A returned rectangle is never proof the target exists. Met. A located box on absent gold counts as a false positive and gets ious[i] is None. It does not change either mean (test_box_returned_for_an_absent_target_is_a_false_positive_never_an_iou). The box is still validated, so a malformed rectangle on absent gold raises instead of being hidden.

Mixed cases I computed by hand

I used gold A = {top .1, bottom .4, left .2, right .6} (area .12) on 5 present examples and 4 absent ones.

Present outcomes:

  • B = {.2, .5, .3, .7}: intersection .06, union .18, IoU 1/3.
  • The whole image: IoU .12.
  • A disjoint box: IoU 0.
  • One sent to review.
  • One said absent.

Absent outcomes: one located box, two reviews, one correct absent.

Quantity By hand Report
mean IoU over located (1/3 + .12 + 0) / 3 = 0.15111 0.15111 (denominator 3)
mean IoU over all present 0.45333 / 5 = 0.09067 0.09067 (denominator 5)
hits at threshold 0.5 / 0.3 / 0.12 0 / 1 / 2 out of 5 0 / 1 / 2 out of 5
false positive rate 1/4 1/4
correct absent rate 1/4 1/4
absent sent to review 2 2
n_total 9 9

I also checked that one present review plus one absent review gives a located mean of n/a (denominator 0), an all-present mean of 0/1, and a correct-absent rate of 0/1. Both agree with the report.

The threshold is inclusive. At 0.12 the whole-image IoU is 0.12000000000000001, so it counts as a hit; that is a float artifact and does not matter.

I probed a few more inputs:

  • np.str_("located") is accepted.
  • An outcome kind that is a list or a dict raises ValueError with the example index.
  • np.bool_(True) for present is rejected. That is right for JSON-sourced labels.
  • A {"box": ..., "confidence": ...} answer mapping is accepted as a located box.

Mutation evidence

I applied each mutation alone to vision.py or fixtures/__init__.py, ran tests/test_vision.py and tests/test_fixtures.py, then restored the file. All 8 were killed:

  • Remove the _check_box_labels call from the validator.
  • Divide the all-present mean by the located count, so reviews drop out.
  • Count absent-gold reviews as correct absents.
  • Count present-gold reviews as misses.
  • Remove the gold area-underflow check.
  • Use > instead of >= for hits.
  • Allow extra keys on an absent label.
  • Divide the hit rate by the located count instead of n_present.

The builder's flagged decisions

  1. A reserved top-level present key declares Box labels. I accept this. A false trigger needs a non-Box recipe whose label object has a top-level present key, and the reservation is documented in docs/fixtures.md ("Box labels"); validate --all shows no current recipe uses one. A recipe that writes Box labels without present anywhere (bare boxes, {"absent": true}) passes the validator, but it fails loudly as soon as it calls localization_report, which runs gold_box on every label. So the gap only shows up as an earlier versus later error, never as a wrong number. A declaration taken from the question type would not work for ranks 83 and 98, which mix types. Suggestion below.
  2. No validation of nested labels (ranks 83 and 98). I accept this. The documented pattern, gold_box(label["target"]) in the recipe's own test plus localization_report([l["target"] for l in ...]), still enforces the format at scoring time. Walking arbitrary nested objects for present keys would bring back the false-trigger risk at every depth.
  3. No review rate printed. I accept this for the Box row, which asks for review outcomes, not a review rate. The counts are on both lines, and docs/evaluation.md points to evaluate_outcomes for the presence question's review rate. Strong suggestion below, because every Box README will quote this number.

Suggestions (non-blocking)

  • Add review_rate to LocalizationReport. Define it as (present_review + absent_review) / n_total, add it to denominators, and print one line, for example "sent to review: 2/9 = 0.2222 (denominator 9: all examples)". Without it, ten recipes will each choose a denominator (n_total, n_present, or resolved-only), which is the exact problem this issue exists to stop. It is an additive line, so the docs test would only need the new line.
  • Add one sentence to docs/fixtures.md "Box labels" saying that a Box recipe that never uses present is not checked by the validator, and that the format is enforced when localization_report or gold_box runs. That sets the right expectation for the 83 and 98 builders.
  • _check_box_labels runs only when the inputs parse (fixtures/__init__.py, the if inputs_ok: block), so Box label errors appear one fix later than other label errors. This is minor and consistent with the existing checks.

Checks run

All checks ran in a fresh venv (pip install -e ".[dev,ml]") with no TYPESAFE_API_KEY.

Check Result
ruff check . All checks passed
ruff format --check . 397 files already formatted
pytest 5392 passed, 58 skipped, 31 xfailed, 50 deselected, 2 failed. The 2 failures are the known Windows-only signal.SIGKILL tests in tests/test_notebook_ci_sandbox.py (#302); this PR does not touch that code.
python tools/render_catalog.py --check rc 0
python tools/check_hygiene.py passed, 606 files
python -m jev_cookbook.fixtures validate --all every recipe valid
Notebook execution not needed: no recipe or notebook references localization_* or gold_box (grep over recipes/)
git diff --raw -M origin/main...HEAD 8 files, all 100644 -> 100644 M; no symlinks, submodules or mode changes. Scope is limited to the issue's listed files.
CI (gh pr checks 331) Lint, Hygiene, Fixtures, Catalog, Scope and discovery passed. Tests and most notebook jobs were still pending when I read them.

No keys, personal data, side effects, or real-quality, latency or cost claims are introduced.

Acceptance criteria (#311)

Criterion Status
Gold Box label format in docs/fixtures.md, vision.gold_box with tests, validator rejects malformed Box labels when declared Met
localization_report(gold, outcomes, *, iou_threshold=0.5) with (kind, box) outcomes: n_total, present and absent tables each counting every example once, both mean IoUs with named denominators, hit rate, false positives, correct absents Met
A printable form naming every denominator; a test showing a reviewed present target changes the stated denominator; hand-computed values Met (test_a_reviewed_present_target_changes_the_denominator_instead_of_vanishing; dyadic rectangles with exact IoUs)
localization_metrics kept, its docs point to the report Met
docs/vision.md and docs/evaluation.md updated; worked example matches the test Met (test_the_documented_localization_example_runs_and_prints_what_the_docs_show runs the docs block and compares stdout byte for byte)

OPUS-REVIEW: APPROVE 34249bf

CompleteDotTech and others added 2 commits October 21, 2026 16:53
…s report (#311 review)

Refs #311

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@CompleteDotTech

Copy link
Copy Markdown
Contributor Author

PR #331 review — reviewer 1 of 2 (independent)

Diff reviewed (git diff c316712...23182da): 8 files, +934/-10 —
src/jev_cookbook/vision.py, src/jev_cookbook/fixtures/__init__.py,
src/jev_cookbook/fixtures/schema.json, docs/vision.md, docs/fixtures.md,
docs/evaluation.md, tests/test_vision.py, tests/test_fixtures.py. No notebook, no
recipe folder, no workflow, no catalog, no root file.

Verdict summary

I found no correctness defect in gold_box, localization_report, LocalizationReport or
the fixture-validator change. The denominators are right and each is named; every example
lands in exactly one table cell; absent and review handling match the issue; the IoU edge
cases (corner-touching, area underflow, threshold inclusivity) are exercised with exact
dyadic values; math.fsum is used for both means and empty denominators are NaN, never 0.
All eight mutations I applied to the head's source were killed by the new tests, including
both failure modes #311 was opened about. Every finding below is a suggestion; none blocks
the merge.

Merge commit (git show --cc HEAD)

git show --cc HEAD prints the commit message and no combined diff hunks at all: the
merge resolved nothing of its own, so it cannot have smuggled in a change. git show --stat HEAD shows what it took from origin/main: tests/test_docs_examples.py (+100/-17),
tests/test_notebook_ci_sandbox.py and tools/execute_notebook.py — PR #325's Windows
portability work, untouched by this branch.

Both branch commits are SSH-signed and verify (git log --format='%G?' → G for 23182da
and 865dfcc). The merge message carries Co-Authored-By: Claude Sonnet 5.5; 865dfcc says
Refs #311, not a closing keyword, which is right for a review-fix commit. No rebase, no
amend, no force-push in the history. git diff --raw -M c316712...HEAD shows all 8 paths as
:100644 100644 ... M — no symlink, submodule, mode or type change, no rename or deletion.
The paths are exactly the issue's "Files likely touched" list plus schema.json, which
changes only a description string. Working tree clean (git status --porcelain empty).

Semantic-merge risk I checked explicitly. origin/main's rewritten
tests/test_docs_examples.py asserts at import time that docs/evaluation.md has exactly
eight fenced python blocks (tests/test_docs_examples.py:84, :170, :174) and executes
each against a hand-written PREAMBLE. This PR adds a ### Box localization section to
docs/evaluation.md consisting of prose and a Markdown table with no fenced python
block, so the count is still 8 (grep -c '^```python$' docs/evaluation.md → 8) and the
merged module is unaffected. The new worked example lives in docs/vision.md, which that
module does not scan; it is covered instead by this PR's own
test_the_documented_localization_example_runs_and_prints_what_the_docs_show. The merge also
happens to clear the two tests/test_notebook_ci_sandbox.py failures the earlier head showed
(#302), so the full suite is green here with nothing skipped.

Correctness review

Denominators, and every example counted once

localization_report (src/jev_cookbook/vision.py:672-752) builds present_table and
absent_table as dict.fromkeys(("located", "absent", "review"), 0) and increments exactly
one cell per example, so n_present + n_absent == n_total == len(targets) holds by
construction with reviews included — there is no filtering step anywhere in the function.
ious is appended exactly once per iteration in both branches, so
len(report.ious) == n_total. overlaps (the non-None entries of ious) is precisely the
located-present set, which is the single structural reason a false positive can never leak
into an IoU mean and a review can never leak into one either.

The five denominators resolve as: mean_iou_located ÷ len(overlaps) (identically
present_table["located"]), mean_iou_present ÷ n_present, hit_rate ÷ n_present,
false_positive_rate and correct_absent_rate ÷ n_absent. denominators
(vision.py:620-629) returns those same five numbers by metric name, and __str__
(vision.py:636-670) prints each one inline with the number it belongs to rather than in a
separate legend, which is the form that actually stops a README from quoting a number without
its denominator. The comment at vision.py:745 is the correct justification for reusing one
sum for both means: unlocated present targets score 0, so the all-present sum is the
located sum.

Absent and review handling

A located outcome on absent gold increments absent_table["located"] (a false positive) and
appends None, so no IoU is invented and neither mean moves; a review never touches
correct_absents or present_missed; a present target called absent is a miss that
contributes a 0 to mean_iou_present only. That is exactly the mapping #311 and
docs/multimodal-track.md:94 ask for. The returned box on absent gold is still passed through
normalized_box, so a malformed false-positive rectangle raises rather than being discarded
silently — tested at tests/test_vision.py ([ABSENT], [("located", {**SAME, "right": 2.0})]).

A present target with a box in the wrong place is located with IoU 0, not a miss. That is
coherent (the box was returned) and is what the dataclass docstring says
(present_located = "a box was returned after the presence check"); FAR covers it and the
test asserts its zero is in the located mean.

IoU edge cases and float behaviour

box_iou can raise only when the union underflows to zero, which needs both areas to
underflow. gold_box (vision.py:346) rejects a gold box whose
(right-left)*(bottom-top) underflows, computed with exactly the same expression box_iou
uses (vision.py:306-310); float rounding to nearest is monotonic, so
intersection <= area_gold and therefore union >= area_gold > 0. The PR body's claim that
box_iou "can never hit #321's zero-union error against a gold box" therefore holds as
stated. test_a_tiny_returned_box_on_a_present_target_scores_zero_without_raising pins the
complementary case — a degenerate predicted box scores 0 and does not raise — and
test_gold_box_rejects_a_degenerate_gold_box_before_any_score_is_computed pins both the
direct and the via-report path with example 1: in the message.

Both means use math.fsum. Empty denominators are float("nan") via _rate and the explicit
if overlaps / if n_present guards, and _share / _mean print
n/a (denominator 0: ...) instead of dividing. The rectangles in the tests and in
docs/vision.md are dyadic (0, 0.125, 0.25, 0.5, 1.0), so IoU 1, 1/2 and 1/4 are exactly
representable and the bare equality assertions (== 0.75, == 0.375, == 0.5, == 1.0,
== 0.0) are legitimate rather than lucky.

iou_threshold must be a non-bool Real, finite, in (0, 1]. Excluding 0 is right: an IoU
of 0 would otherwise count as a hit. The comparison is >=, and
test_report_hit_threshold_is_inclusive_and_configurable proves inclusivity on an IoU of
exactly 1/4 against a threshold of 0.25 — both exact, so that test genuinely distinguishes
>= from > rather than relying on rounding.

The fixture-validator change

_check_box_labels (src/jev_cookbook/fixtures/__init__.py:556-581) indexes label_lines,
which _read_labels (:349-359) populates in lockstep with labels inside the same loop
body, so label_lines[ident] cannot KeyError — I traced that specifically because it is the
obvious way such a check breaks. The deferred from ..vision import gold_box is not a cycle
(jev_cookbook.vision imports only ._canonical, .evaluation, .style, none of which
reach fixtures).

No recipe on main is affected: no labels.jsonl under recipes/ contains a top-level
present key (checked across all 51 folders), and recipe 35's object labels are
{"features": {...}, "escalate": ...}, which does not declare. So this check cannot turn a
currently green recipe red. schema.json changes only a description string.

API shape for recipes

The outcome triple matches the localization_outcome policy helper already printed at
docs/vision.md:140-153 exactly ("review", None / "absent", None /
"located", normalized_box(answer)), so a recipe pipes one straight into the other with no
adaptation — that coherence is the main thing I was looking for and it is there.
localization_report also accepts a raw BoxAnswer-shaped object for the located box (via
normalized_box's .box handling), proved by
test_report_accepts_lists_a_box_answer_object_and_any_iterable, and accepts arbitrary
iterables and lists-instead-of-tuples. This is a copyable interface.

Do the tests prove their rules? (mutation probe)

I ran eight deliberate mutations of the head's own source, one at a time, restoring the file
after each (/home/agent/r1-331-mutate.py, pytest -x over tests/test_vision.py and
tests/test_fixtures.py; git status --porcelain empty afterwards). Every one was killed:

Mutation applied to the head Result
M1 present_table["review"] = 0 — reviews vanish from the present denominator test_report_mixed_outcomes_by_hand failed (assert (4 + ...)
M2 hits uses > instead of >= test_report_mixed_outcomes_by_hand failed (assert 1 == 2)
M3 a box on absent gold counted as a correct absent test_report_mixed_outcomes_by_hand failed
M4 mean_iou_present divided by the located denominator test_report_mixed_outcomes_by_hand failed (assert 0.58...)
M5 gold_box drops the zero-area underflow check test_gold_box_rejects_malformed_labels[label26] failed
M6 gold_box accepts a truthy non-boolean present test_gold_box_rejects_malformed_labels[label7] failed
M7 _check_box_labels returns [] unconditionally test_malformed_box_labels_are_rejected[bad0] failed
M8 n_total excludes reviewed examples test_report_mixed_outcomes_by_hand failed (assert (5 + ...)

M1 and M8 are the two failure modes #311 was opened about ("a present target sent to review
silently leaves mean_iou's denominator", "no n_total that includes reviews"), and M7 is the
new validator rule — so the new guard demonstrably fails on cases where it must. The
complementary direction is also covered without my help:
test_labels_without_a_present_key_are_not_treated_as_box_labels and
test_well_formed_box_labels_present_and_absent_are_valid assert messages(recipe) == [], so
an over-eager validator would be caught too.

Must-change findings

None.

Suggestions (non-blocking)

  1. src/jev_cookbook/vision.py:710, 724 — guess leaks across loop iterations. It is
    assigned only inside if kind == "located" and read only in the matching
    if kind == "located" branch of the same iteration, so the code is correct today. But
    nothing keeps it that way: an edit that read guess outside that guard would silently
    score the previous example's rectangle instead of raising UnboundLocalError. Compute
    the IoU inside the located branch, or set guess = None at the top of each iteration, so
    the failure mode is a crash rather than a wrong number in a report whose whole purpose is
    that numbers cannot go quietly wrong.
  2. src/jev_cookbook/vision.py:724 — box_iou is called outside the try, so a raise from
    it would lose the example N: prefix
    the docstring and docs/vision.md promise. It is
    unreachable today, precisely because gold_box guarantees a non-underflowing gold area,
    but moving the call inside the guarded block costs nothing and keeps the promise true
    independently of that invariant.
  3. src/jev_cookbook/vision.py:694, 741 — hits compares against the uncoerced
    iou_threshold while the report stores float(iou_threshold).
    Any non-float Real
    (a fractions.Fraction, a numpy scalar) can therefore be applied at one precision and
    reported at another, which is a small version of exactly the problem this report exists
    to prevent. Coerce once (threshold = float(iou_threshold)) and use that value for both
    the comparison and the stored field.
  4. src/jev_cookbook/fixtures/__init__.py:573-579 — the misfire message does not mention
    the reservation.
    The declaration mechanism is "a top-level present key", which
    docs/fixtures.md documents as reserved. A recipe that is not a Box recipe but happens
    to label with, say, {"present": true, "severity": "high"} now gets
    malformed Box label: a present gold Box label must be exactly 'present' and 'box', which
    tells the author nothing about the reservation or that nesting is the way out. Say so in
    the message and point at docs/fixtures.md "Box labels". I am not treating the reserved
    key itself as a blocker: it is documented, the PR body raises it openly for reviewers, no
    recipe on main is affected, and every failure it can cause is loud.
  5. src/jev_cookbook/fixtures/__init__.py:565 — nested Box labels get no validator
    coverage, although the PR's own test helper shows the recursion is three lines.

    tests/test_vision.py:1164-1175 (_box_label_objects) walks a label and validates every
    dict with a present key — exactly what the validator declines to do. Consider having
    _check_box_labels do the same walk for object labels, so the rank-98 mixed-type shape is
    checked mechanically instead of by a per-recipe test the contract cannot enforce (the
    train split in particular never passes through localization_report). The docs disclose
    the gap honestly, which is why this is a suggestion rather than a must-change, and I
    acknowledge the counter-argument that recursing would reserve present at every depth.
  6. src/jev_cookbook/fixtures/__init__.py:721-725 — the Box-label check is nested under
    if inputs_ok: although it reads nothing from the inputs.
    A recipe with an unreadable
    inputs.jsonl and malformed Box labels is told only about the inputs, so the author
    needs two rounds. It cannot produce a false pass (the inputs problem already fails the
    recipe), but hoisting the call next to if labels_ok: would report both in one run.
  7. src/jev_cookbook/fixtures/__init__.py:569 — the deferred-import comment's stated reason
    does not hold.
    "deferred: validating other recipes needs no Pillow" — Pillow is a hard
    dependency (pyproject.toml:11-18), so it is always installed and the deferral cannot save
    anyone an install. It does save the import cost; say that instead, so a later reader does
    not conclude the import is optional and restructure around a false premise.
  8. docs/vision.md:244-247 — the error sentence overreaches. "Malformed gold, an unknown
    outcome kind, a located outcome without a valid box, a box attached to absent or
    review, empty or unequal-length inputs and an iou_threshold outside (0, 1] raise
    ValueError naming the example index" — the last two raise before any example is
    examined and name no index. The function's own docstring (vision.py:681-685) is careful
    to keep the two groups separate; the document that ten Box recipes copy should be too.
  9. src/jev_cookbook/vision.py:553 — the outcome kinds are private (_OUTCOMES). Ten
    recipes will hardcode the string literals "located", "absent", "review", and the
    error message f"outcome kind must be one of {_OUTCOMES}" points at a name a reader cannot
    import. Exporting the tuple in __all__ would make both referenceable.
  10. docs/fixtures.md:124-135 — show the one line a nested-label recipe needs in order to
    report.
    The section explains that such a recipe calls gold_box(label["target"]) in
    its own test, but localization_report takes labels, not decoded boxes, so the reporting
    call is localization_report([label["target"] for label in labels], outcomes). One line
    would stop ten recipes from guessing — and in particular from trying to pass the decoded
    None, which gold_box rejects with "a gold Box label must be an object".
  11. No n_review / review_rate on the report. present_review + absent_review is the
    first number a reader of a "nothing disappears" report reaches for, and without it each
    README picks its own denominator — the exact failure Foundation: Box evaluation can drop reviewed and absent cases from the IoU denominator, and there is no label format for an absent target #311 exists to stop.
    docs/evaluation.md:762 does route the reader to evaluate_outcomes for the presence
    question's review rate, which is a denominator-safe answer, and the issue does not ask for
    this, so it is a suggestion. I note that both prior reviewers raised it independently, so
    three reviewers now agree it is worth a follow-up.

Acceptance criteria of issue #311

  1. Gold Box label format in docs/fixtures.md, plus vision.gold_box(label) decoding and
    validating geometry, with tests; the fixture validator rejects malformed Box labels when a
    recipe declares Box labels.
    — Met. docs/fixtures.md:105-141 ("Box labels") gives the
    format and its rules; gold_box (vision.py:320-349) decodes and validates; 27
    malformed-label cases in test_gold_box_rejects_malformed_labels, 12 in
    test_malformed_box_labels_are_rejected, plus the two declaration-boundary tests in
    tests/test_fixtures.py. Recorded as suggestions 4 and 5: the declaration is the reserved
    top-level present key, and a nested Box label is not validated.
  2. localization_report(gold, outcomes, *, iou_threshold=0.5) taking ("located", box) /
    ("absent", None) / ("review", None), returning n_total; a present × outcome and an
    absent × outcome table counting every example exactly once; mean IoU over located-present
    and over all present with missed/review as 0, each with its denominator named; the hit rate
    at the threshold; false positives on absent targets; correct absents.
    — Met.
    vision.py:672-752 and LocalizationReport (:567-670): present_table, absent_table,
    mean_iou_located, mean_iou_present, hit_rate, false_positives, correct_absents,
    absent_review, denominators.
  3. A printable form that names every denominator; tests show a reviewed present target
    changes the stated denominator rather than vanishing, using hand-computed values.
    —
    Met. __str__ prints all five denominators inline;
    test_a_reviewed_present_target_changes_the_denominator_instead_of_vanishing holds gold
    fixed and asserts mean_iou_located 1.0 at denominator 1 against mean_iou_present 0.5 at
    denominator 2, then asserts both strings appear in str(report). The comment block at
    tests/test_vision.py:875-883 derives every IoU by hand from the four rectangles. My M1
    and M8 mutations confirm those tests fail when a review vanishes.
  4. localization_metrics kept for compatibility; its docs point to the report as the
    recommended entry.
    — Met. vision.py:513-520; the body is unchanged (only the
    docstring grew), the pre-existing tests still pass, and
    test_report_is_a_frozen_dataclass_and_localization_metrics_remains asserts both the
    docstring pointer and that the old function still computes. docs/vision.md:269-274 keeps
    it as "The older localization_metrics".
  5. docs/vision.md and docs/evaluation.md updated; the worked example matches the test. —
    Met. docs/vision.md:157-247 (gold labels, the report, the worked example, the tiled
    section) and docs/evaluation.md:739-769 (the denominator table and the tiled paragraph).
    test_the_documented_localization_example_runs_and_prints_what_the_docs_show extracts the
    fenced block from docs/vision.md, execs it and compares stdout with the text fence
    byte for byte, so the example cannot drift from the code.

PR body against the diff

Accurate everywhere I could check it, including every reported number (see below). The design
note discloses both real gaps — the nested-label blind spot and the reserved-key declaration —
instead of hiding them, and invites reviewers to object, which is the right way to handle a
decision ten recipes will inherit. "No notebook touches vision.localization_*" is confirmed:
no .ipynb under recipes/ references localization_metrics, localization_report or
gold_box, so no notebook execution is affected and none was needed. No overclaim found; no
"fixed" statement that the diff does not support. No secret, no personal data, no absolute home
path, no statement about Jev's quality, latency or cost (CONTRIBUTING.md §2), no side effect.
Branch name and Closes #311 match CONTRIBUTING.md §"Branches, commits, pull requests"; the
recipe-scope allowlist does not apply (foundation branch, #311 is not a catalog recipe issue),
and Scope (recipe pull requests) passes on the head.

Checks run

Head 23182da, Linux, fresh Python 3.10.21 venv at /home/agent/venv-pr331
(uv venv --python 3.10; uv pip install --link-mode symlink -e . plus ruff==0.17.1 and
pytest), no TYPESAFE_API_KEY, everything under
flock .claude/heavy.lock nice -n 10 and pytest -p no:cacheprovider:

Check (docs/development.md) Result PR body claim
ruff check . All checks passed! (exit 0) matches
ruff format --check . 397 files already formatted (exit 0) matches
pytest 5409 passed, 49 skipped, 50 deselected, 31 xfailed in 287.74s (exit 0) matches exactly
python tools/check_hygiene.py Hygiene check passed: 606 file(s) scanned. (exit 0) matches
python tools/check_recipe_assets.py Image-asset check passed: 461 file(s) under recipes/ checked. (exit 0) matches ("passed")
python tools/render_catalog.py --check exit 0, no output matches
python tools/notebook_ci.py fixtures 51 recipe folders, 0 failed (exit 0) matches
ruff format --check on the 3 changed docs/*.md 7 files already formatted — the fenced Python in docs/vision.md is format-clean —
Mutation probe (8 mutations, each applied alone and reverted) all 8 killed; tree clean afterwards —

Nothing was skipped, loosened or disabled to get there; no fixture was edited. Notebook
execution: not applicable and not performed — no notebook or recipe references the changed
API (grep over recipes/**/*.ipynb and recipes/**/*.py), and the diff touches no
recipes/ path, so no notebook's behaviour can change.

Independent corroboration: all 61 GitHub checks on exactly this head are green
(gh pr checks 331; runs 39644885382 and 39644885315 both report
head_sha 23182da7..., conclusion success), including Tests (py3.10), Tests (py3.14),
Lint (ruff), Hygiene, Fixtures (validate), Catalog (README is current),
Scope (recipe pull requests) and all 52 notebook jobs.

Prior review findings (read only after the above was written)

Both prior reviews are on the earlier head 34249bf against base e998eb5, i.e. before both
865dfcc and the merge of c316712.

Comment 6306168056 (prior reviewer 1) — verdict line: approve, on 34249bf. It listed no
blocking findings
, so there is nothing of that class to resolve. Its three non-blocking
suggestions at this head: review_rate on the report — not done (I re-raise it as my
suggestion 11); a sentence in "Box labels" saying a Box recipe that never writes present is
unchecked by the validator and the format is enforced when gold_box/localization_report
runs — not done (reasonable and still worth adding; related to my suggestion 5);
_check_box_labels running only when the inputs parse — not done, and we independently
reached the same minor conclusion (my suggestion 6). Its pytest run showed 2 failures in
tests/test_notebook_ci_sandbox.py (the Windows-only SIGKILL tests, #302): the merge of
c316712 into this head is exactly PR #325's fix for those, and my full run at this head is
clean, so that observation is now moot.

Comment 6306161479 (prior reviewer 2) — verdict line: changes needed, on 34249bf, with two
blocking findings, both in the docs. For each:

  1. "docs/fixtures.md:129-132: the nested-label example is contradictory, and rank 83 will
    copy it"
    — the old example was {"tile": "b3", "target": {"present": false}}, which
    names a gold tile for a target that is not in the image, and teaches storing a tile choice
    beside the box. Resolved at this head. 865dfcc replaces it with
    {"id": "t12", "label": {"target": {"present": true, "box": {...}}, "rubric": 3}} (the
    rank-98 shape the reviewer suggested), and adds the paragraph "Store the gold box once, for
    the whole image, as a full-image Box label. A recipe that tiles the image does not store a
    tile choice beside it…", which gives both of the reviewer's reasons: for an absent target
    there is no right tile, and with overlapping tiles a target near a seam lies in more than
    one tile. It is also now enforced, not just written:
    test_every_label_example_in_the_box_labels_docs_is_valid_and_consistent parses every JSON
    example in the section, runs gold_box on every nested object with a present key, asserts
    no example contains any key matching tile, and asserts "tile": does not occur anywhere
    in the section. I verified that test passes and that it is the reason a regression could not
    slip back in. Resolved.
  2. "docs/vision.md does not say what one example is when a recipe tiles the image" —
    Resolved at this head. 865dfcc adds ### Tiled and hierarchical recipes
    (docs/vision.md:249-266) carrying all three things the reviewer asked for: the headline
    report is per image, against the full-image gold, computed after tile_to_image; stage-1
    "no match" on present gold maps to ("absent", None) and an uncertain choice to
    ("review", None); a per-tile report is a stage diagnostic, must be labelled as counting
    tiles, must never be quoted as the recipe's result, and must state its rule for a target a
    tile edge cuts (naming the minimum-visible-fraction rule explicitly). The reviewer's
    alternative of a docs/evaluation.md row was also taken: docs/evaluation.md:764-769
    repeats it and links the section. Again enforced, by
    test_the_docs_say_a_tiled_recipe_reports_per_image_not_per_tile, which pins seven required
    phrases in docs/vision.md plus the docs/evaluation.md sentence and the cross-link from
    both evaluation.md and fixtures.md. Resolved.

Prior reviewer 2's non-blocking suggestions at this head: "say explicitly that the test must
cover every split" — done ("so that test must cover every split the recipe ships (train
included), not only the one it reports", with "must cover every split" asserted by the docs
test); an image_to_tile helper — not done, and the reviewer said it does not belong in
this PR, with which I agree; a sentence in "Box labels" about ambiguous targets (two identical
buttons: the criteria must pick out one rectangle and a review is the honest cost of
abstaining) — not done, worth a follow-up but genuinely non-blocking; an error message
that names the present reservation — not done, and I reached the same suggestion
independently (my suggestion 4); n_review — not done (my suggestion 11).

So both of the prior round's must-change findings are resolved at this head, each with a test
that would catch a regression, and no prior finding of any severity has been resolved in a way
that introduced a new problem. The remaining unaddressed items are suggestions in all three
reviews, including both prior reviewers' own judgement.

Outside my task (reported, not fixed)

  • /workspace was full when I started (0 MB free; tool output was being lost to ENOSPC),
    not the ~450 MB my brief expected. I removed one stale harness scratch directory,
    /home/agent/.cache/opencode-tmp/claude-1000/-workspace-…-projects-gh-runs-2026-10-06
    (7.9 GB, last written 2026-10-08), which freed 8 GB and unblocked the review. That directory
    is harness temp scratch from a 13-day-old session, not anyone's work product.
    /home/agent/.cache/opencode-tmp still holds ~1.8 GB of other stale session scratch and
    ~/.cache/ms-playwright 1.3 GB; neither is mine to delete. Nothing in the repository, the
    primary checkout or any other agent's worktree was touched, and this had no effect on the
    head under review.
  • Pre-existing, not this PR: src/jev_cookbook/vision.py:525 says of
    localization_metrics "Invalid rectangles raise, including predictions on absent gold
    examples", which reads as though a prediction on absent gold raises. It does not — it is
    counted as a false positive; only an invalid rectangle raises, wherever it appears. This
    PR leaves the line untouched (correctly, being out of scope); worth a wording fix in its own
    pull request.

Conclusion

The interface is the one #311 specified, it is coherent with the localization_outcome policy
the same document already teaches, its denominators are correct and named in both the printed
form and denominators, and its tests demonstrably fail on the cases they claim to cover —
including the two that motivated the issue. Both must-change findings from the previous round
are resolved, and each is now held in place by a test. All eight docs/development.md checks
pass locally at this head with nothing skipped or loosened, and all 61 GitHub checks pass on
the same SHA. My eleven suggestions are improvements to a sound change, not conditions; I
would be comfortable with this merging as it stands.

OPUS-REVIEW: APPROVE 23182da

@CompleteDotTech

Copy link
Copy Markdown
Contributor Author

PR #331 review (independent review 2 of 2)

I formed the view below from the diff, issue #311, CONTRIBUTING.md, docs/multimodal-track.md
and the catalog, and only afterwards read the two prior reviews (comments 6306168056 and
6306161479, both on head 34249bf). Their status is in its own section at the end.

Verdict summary

The code is correct, the tests are strong and genuinely hand-computed, the new validator check
demonstrably fires on the cases it must and demonstrably stays silent exactly where the docs say
it will, the two new docs-structure guards demonstrably fail when the statements they pin are
removed, every check from docs/development.md passes, the scope is clean, and the PR body
matches the diff and my own measured numbers exactly. I have no blocking findings. The nine
findings below are documentation precision and follow-ups; none would make a recipe compute a
wrong number. If the docs are opened again for any reason, F1-F4 are the lines to fix.

Must change

None.

Findings (all non-blocking, ordered by severity)

F1. docs/vision.md:238-242 - "naming the example index" is untrue of two of the six error classes it is attached to

"Malformed gold, an unknown outcome kind, a located outcome without a valid box, a box
attached to absent or review, empty or unequal-length inputs and an iou_threshold outside
(0, 1] raise ValueError naming the example index."

The first four do carry example N: (the wrapper at src/jev_cookbook/vision.py:698-714). The
last two are raised before the loop and carry no index at all. Measured at this head:

localization_report([], [])                            -> "gold and outcomes must be nonempty and have the same length"
localization_report([ABSENT], [])                      -> "gold and outcomes must be nonempty and have the same length"
localization_report([ABSENT], [...], iou_threshold=0)  -> "iou_threshold must be a number in (0, 1]"

Change: split the sentence, e.g. "... raise ValueError naming the example index. Empty or
unequal-length inputs and an iou_threshold outside (0, 1] raise ValueError before any example
is scored." This is the one sentence a recipe author reads when deciding what to catch and what
to print, so its precision is worth more than usual.

F2. docs/fixtures.md:107-108 - "ten recipes" is nine

"A recipe whose question asks for a Box labels each example with a gold Box label, so ten
recipes do not invent ten formats."

catalog/recipes.json has exactly nine entries whose decision_types include Box (ranks 77,
78, 79, 80, 81, 82, 83, 84, 98) - the same nine #311 lists under "Catalog ranks". The "ten" comes
from #311's prose, not from the catalog. Change to "nine", or drop the count ("so every Box recipe
does not invent its own format"), which cannot go stale when the catalog changes.

F3. docs/fixtures.md:409 - the reflowed sentence paraphrases a Box label as "a normalized box"

"Labels can contain the gold Box label above (a normalized box, or the explicit
{"present": false})."

A bare normalized box is precisely what the new format does not accept, and what
_check_box_labels now reports as "label is not a Box label" once a sibling declares the format.
Someone reading only this line writes {"top": ..., "bottom": ...} as the label. Change the
parenthetical to name the shape, e.g. "({"present": true, "box": {...}}, or the explicit
{"present": false})". The same edit fixes the reflow: this line is now 117 columns where the
paragraph around it wraps at about 95. Nothing enforces a prose width (docs/evaluation.md has
longer prose lines on main), it is just visibly different from its neighbours.

F4. docs/vision.md:254-256 and docs/evaluation.md:766-767 - the stage-1 mapping is phrased as if it were conditioned on gold

vision.md: "a stage-1 "no match" on an image whose gold is present is ("absent", None) (a miss)"

evaluation.md: "with a stage-1 "no match" on present gold reported as absent"

The mapping from a pipeline result to an outcome must never look at gold. The framing sentence
("Map the stage-1 result to an outcome before reporting") is right and the clause is describing
the consequence when the gold happens to be present - but as written the gold condition sits
inside the mapping rule, and this is the paragraph ranks 80 and 83 will copy. Change to the
gold-blind form: "a stage-1 "no match" is ("absent", None) whatever the gold is - a miss when
the gold is present, a correct absent when it is not". The ("review", None) half of the
sentence is already gold-blind and needs no change.

F5. src/jev_cookbook/fixtures/__init__.py:556-581 - the validator cannot see nested Box labels, and the recursion it would need already exists in the test file

This is the documented limitation (docs/fixtures.md:126-132), and I confirmed it on a real
recipe copy: with every label nested as {"target": {"present": true, "box": {"top": 2, "bottom": 3}}, "rubric": 3} and no top-level present anywhere, validate_recipe reports
nothing. The consequence is that coverage is inverted with respect to risk: ranks 77-79, 81, 82
(Box + Noul, whose gold is present + box) get the check, while the ranks most likely to
need sibling gold - 80 (Box + Choice), 83 (tiled), 84 (per-edge verification questions) and
98 (all four decision types) - must nest and get no validator coverage at all, falling back to a
convention (gold_box(label["target"]) in the recipe's own test) that nothing enforces.

tests/test_vision.py:1165-1175 already contains exactly the recursion that would close this
(_box_label_objects: every dict under the value, itself included, that has a present key).
Hoisting that shape into _check_box_labels would cover all nine ranks and would make the "must
cover every split" instruction unnecessary. The trade-off is that the false-trigger surface of the
reserved present key grows from the top level to every depth, which is a real cost and a
judgement call - hence a suggestion, not a requirement. Worth settling before the first tiled
recipe is built rather than after.

F6. src/jev_cookbook/fixtures/__init__.py:722-725 - Box-label problems are gated on inputs_ok

_check_box_labels is called inside if inputs_ok: although it reads only labels and
label_lines. An unreadable inputs.jsonl therefore hides every malformed Box label until the
inputs are fixed. Nothing is weakened (validation still fails), it just costs an extra round. It
is consistent with the existing _check_examples placement, so moving it out is optional; prior
reviewer 1 noted the same.

F7. src/jev_cookbook/vision.py:341 - the error for an extra sibling key does not mention the nesting rule

Measured on a recipe copy whose label is {"present": true, "box": {...}, "rubric": 3}:

labels.jsonl:2 (id '...'): malformed Box label: a present gold Box label must be exactly 'present' and 'box'

That is the exact message a rank 84 or 98 author hits the first time they add sibling gold, and it
does not say what to do. Suggest: "... must be exactly 'present' and 'box'; nest it under your own
key to store other values beside it". The same applies to the reserved-key case (a non-Box recipe
whose label object happens to carry a top-level present): say that present is reserved for Box
labels, so the author knows to rename. Prior reviewer 2 asked for this wording too; it is still
absent.

F8. src/jev_cookbook/fixtures/__init__.py:569 - the deferred-import comment gives a reason that is not true

from ..vision import gold_box # deferred: validating other recipes needs no Pillow

Pillow is always installed: matplotlib is a core dependency (pyproject.toml) and depends on
it, so import jev_cookbook.vision cannot fail for want of Pillow in any supported environment.
The deferral is still worth keeping - it keeps jev_cookbook.fixtures from pulling Pillow, numpy
and the style module into every validator run - so state that reason instead.

F9. docs/multimodal-track.md:94 is still the only place a Box author is told what to report, and it names no helper

The Box row of "What an evaluation must show" is what every Box builder reads first. This PR
satisfies it, but the row names no function, so an author can read it and never discover
localization_report. One clause ("report it with vision.localization_report; see
vision") would close the loop. Outside the files #311 lists,
so a follow-up is fine.

Observations, not findings

  • No image_to_tile inverse of tile_to_image exists, yet docs/fixtures.md:142-143 tells a
    tiled recipe that "Python derives which tiles count as correct from the box and the tile bounds"
    and docs/vision.md:257-260 tells it to state a minimum-visible-fraction rule. Each tiled recipe
    therefore writes that clipping itself, and edge-snapping mistakes are likely. Correct for this
    PR's scope (Foundation: Box evaluation can drop reviewed and absent cases from the IoU denominator, and there is no label format for an absent target #311 is the label format and the report); worth a foundation issue before rank 83 is
    built. Prior reviewer 2 raised the same point and the author rightly left it out.
  • Neither LocalizationReport.denominators nor str(report) carries a single review total
    (present_review + absent_review) or a review rate. Both prior reviewers suggested adding one;
    it is still absent. I do not think it blocks: the printed report labels both review cells with
    their own n, and docs/evaluation.md:763 points at evaluate_outcomes for the presence
    question's review rate. But it is the one number a Box README will want and cannot read off the
    report, so it will be derived nine times by hand.
  • Out of scope, pre-existing on main: docs/development.md ("Optional pre-commit") says the
    pinned ruff is ruff==0.16.10, while pyproject.toml pins ruff==0.17.1. Not this PR's doing
    and not in its diff - reporting it rather than smuggling a fix in.

What I verified myself

The report's arithmetic

Independent of the author's rectangles, with gold A = {top .1, bottom .4, left .2, right .6}
(area .12) and with the documented example, I re-derived every number from the printed report and
from the dataclass: mean_iou_located = (1.0 + 0.5)/2 = 0.75, mean_iou_present = 1.5/4 = 0.375,
hit_rate = 2/4 = 0.5, false_positive_rate = correct_absent_rate = 1/3. Each example lands in
exactly one table cell, and n_present + n_absent == n_total == len(ious) in every case I built.

Structural properties I checked in the code rather than by example:

  • len(overlaps) == present_located always: an IoU is appended only for present gold with a
    located outcome (vision.py:715-721), so mean_iou_located's divisor and the denominator
    printed by __str__ / returned by denominators cannot drift apart.
  • mean_iou_present = math.fsum(overlaps) / n_present is exactly "missed and reviewed count 0",
    with the one-line comment at vision.py:724 saying why the sum is the located sum.
  • gold_box's area check (vision.py:346-347) uses the same product expression as box_iou's
    area_a (vision.py:310-311), so a gold box accepted by gold_box has area > 0, hence
    union >= area_gold > 0, hence box_iou can never reach Foundation: enforce the Images API's 280 px minimum, hash image bytes in replay keys, bound tiles (#306) #321's zero-union error inside the
    report. The PR body's claim holds. A returned box that underflows scores 0 instead of raising
    (test_a_tiny_returned_box_on_a_present_target_scores_zero_without_raising) - correct.
  • gold_box rejects {"present": true, "box": {"box": {...}}} because it checks
    set(raw) != set(_EDGES) before calling normalized_box, whose box-key unwrapping would
    otherwise accept it. Deliberate and right: the gold side is strict, the prediction side stays
    lenient through normalized_box, so a BoxAnswer or a raw answer mapping works as the outcome.
  • iou_threshold excludes 0 (0 < t <= 1), which is right - at 0 a disjoint box would count as a
    hit. Rejecting bool before Real stops iou_threshold=True being read as 1.0.

The new guard fires where it must (on a real recipe, not only in tmp fixtures)

I copied recipes/01-sentiment-classification to a scratch directory outside the checkout and ran
validate_recipe on mutated labels:

Labels Result
untouched [] (baseline)
all well-formed Box labels, mixed present/absent []
one inverted box (top .9 > bottom .1) labels.jsonl:2 (id 'v02-positive'): malformed Box label: box must have positive width and height
one "billing" string among Box labels labels.jsonl:2 ...: label is not a Box label ({'present': ...}) but this recipe's other labels are
{"present": true, "box": {...}, "rubric": 3} labels.jsonl:2 ...: malformed Box label: a present gold Box label must be exactly 'present' and 'box'
all labels nested under target, well-formed [] (documented limitation, F5)
all labels nested under target, box {"top": 2, "bottom": 3} [] (documented limitation, F5)

The two new docs guards fail when the statements they pin are removed

In my own worktree (restored immediately afterwards; git status --porcelain empty and the head
unchanged) I reintroduced the two things prior reviewer 2 blocked on:

  • put {"tile": "b3", "target": {"present": false, "box": {...}}, "rubric": 3} back as the nested
    example -> test_every_label_example_in_the_box_labels_docs_is_valid_and_consistent FAILED
    with ValueError: an absent gold Box label must be exactly {'present': false};
  • changed "one example is one image, not one tile" to "each tile is scored on its own" ->
    test_the_docs_say_a_tiled_recipe_reports_per_image_not_per_tile FAILED on that string.

So both regressions the prior round found are now mechanically prevented, not merely reworded.

The documented example really runs

Extracting the python block of docs/vision.md and comparing stdout with the following
text block byte for byte, outside pytest: match. The three JSON label examples in
docs/fixtures.md "Box labels" all decode (t07 -> the rectangle, t08 -> None, t12 -> the
rectangle via label["target"]).

Scope and PR body

git diff --raw -M origin/main...HEAD:

:100644 100644 47829e3 f9d257d M	docs/evaluation.md
:100644 100644 b0f5b89 c7530ab M	docs/fixtures.md
:100644 100644 f9ebcad 45e2415 M	docs/vision.md
:100644 100644 1fd2cec ab4a52c M	src/jev_cookbook/fixtures/__init__.py
:100644 100644 7265f2c 62d7d82 M	src/jev_cookbook/fixtures/schema.json
:100644 100644 6d5148c 959bfe9 M	src/jev_cookbook/vision.py
:100644 100644 55a6aaa bc814fa M	tests/test_fixtures.py
:100644 100644 e2a31f6 fe139f1 M	tests/test_vision.py

Eight files, every one M with mode 100644 -> 100644. No symlink (120000), no submodule
(160000), no mode change, no type change, no rename, no addition or deletion. Nothing under
recipes/, .github/, catalog/, and no README.md change. These are the files #311 lists plus
schema.json, which changes one description string only.
tools/check_recipe_scope.py --base origin/main --head HEAD --branch foundation/box-evaluation --body-file ... -> "scope ok: foundation pull request: the recipe allowlist does not apply".

PR body against the diff: every bullet is true of the code. Closes #311 appears once and is the
only closing keyword anywhere in the body; the other issue references (#321, #306) are plain
mentions. Every verification number I could re-measure matches exactly: ruff format --check 397
files, pytest 5409 passed / 49 skipped / 50 deselected / 31 xfailed, hygiene 606 files, fixtures
51 folders / 0 failed. "No notebook touches vision.localization_*" is true -
grep -rln "localization_metrics\|localization_report\|gold_box" recipes/ is empty, so no notebook
execution is affected and none was needed. No key, no credential, no personal data, no absolute
home path, and no claim about Jev's real quality, latency or cost.

Fit with the track's actual shapes (docs/multimodal-track.md and the catalog)

  • docs/multimodal-track.md:94, the Box row ("IoU against labeled rectangles on present
    targets, plus separate absent-target and review outcomes. State the denominator so rejected or
    absent cases cannot disappear from the report"): satisfied in full. An IoU is appended only for
    present gold with a located outcome; present and absent each get their own three-cell row;
    every mean and rate prints "(denominator N: what)" and is reachable by name through
    denominators; a zero denominator is NaN printing n/a, never 0.
  • Ranks 77, 78, 79, 81, 82 (Box + Noul): the natural label is {"present": ..., "box": ...}, presence gold is gold_box(label) is not None, and the documented localization_outcome
    policy (docs/vision.md:143-152) already returns exactly the (kind, box) pairs the report
    takes. Clean fit, with validator coverage.
  • Ranks 80 (Box + Choice), 84 (per-edge verification), 98 (all four types): need sibling
    gold, so they must nest and lose validator coverage (F5). The report itself fits - each compared
    method of rank 98 gets its own localization_report against the same gold, and ious has one
    entry per example, so methods can be compared example by example.
  • Rank 83 (tiled / hierarchical): covered by the new section. Gold stored once, full-image;
    the outcome is the mapped rectangle after tile_to_image; stage-1 "no match" -> absent;
    uncertain -> review; a per-tile report is a labelled stage diagnostic that must state its
    tile-edge rule. I checked the arithmetic of the per-tile alternative the docs warn against: with
    tiles as examples the empty tiles land only in the absent table, so they do not inflate either
    mean, but n_total becomes a tile count and correct_absent_rate mostly measures empty tiles -
    exactly what the docs now forbid quoting as the recipe's result.
  • Absent targets: {"present": false} is a first-class label, null stays forbidden by
    schema.json, and a located box on absent gold is a false positive with ious[i] is None.
  • Uncertain / review: never an absent prediction and never a correct absent
    (test_report_all_review_keeps_every_example_in_the_denominator asserts
    correct_absents == false_positives == present_missed == 0 with everything reviewed).
  • Consistency with the image rules on main (Foundation: vision.py accepts 256–279 px images that the Images API rejects; fix limits and close #297's carried-over geometry and key defects before the first image fixture #306 / Foundation: no contract or check for a recipe's image files (location, format, size, metadata), and the hygiene, scope and freshness checks treat binary assets inconsistently #307 / Foundation: enforce the Images API's 280 px minimum, hash image bytes in replay keys, bound tiles (#306) #321): the only pre-existing
    sentence touched is docs/vision.md:120-127, and its new clause is true - gold_box rejects a
    degenerate gold label up front, so box_iou's zero-union error can no longer be reached from a
    gold box. Nothing contradicts the 280 px minimum, the image-file contract, the asset checker or
    the tile bound, and the new tiled-recipe text adds no image rule of its own.

Test quality

The hand-computed claim holds: every rectangle is dyadic (GOLD area 0.25, plus SAME, HALF,
QUARTER, FAR), so the IoUs are exact, the comment block at tests/test_vision.py:876-881
derives each one, and test_report_mixed_outcomes_by_hand asserts the whole dataclass including
ious and denominators. counted_once() is applied as an invariant across the cases.
test_a_reviewed_present_target_changes_the_denominator_instead_of_vanishing
(tests/test_vision.py:1045-1066) is exactly the test #311's third criterion asks for: the same
gold, one outcome changed from located to review, and both the located denominator (2 -> 1) and
the all-present denominator (2, unchanged) asserted, plus the printed strings. Threshold
inclusivity (>= at exactly 0.5), iou_threshold=1, all-absent, all-review, a located box on
absent gold, present-called-absent, a degenerate gold label, a tiny returned box, 16
malformed-input shapes with the example index, 8 bad thresholds, plain iterables and a
SimpleNamespace answer are all covered.

Checks run

All under flock .../.claude/heavy.lock nice -n 10, in a venv created for this review at
/home/agent/venv-pr331-r2 (uv venv --python 3.10, then
uv pip install --link-mode symlink -e . ruff==0.17.1 pytest, which is .[dev] by its contents -
the sandbox refused the bracket form on a flock command line). I confirmed before running
anything that jev_cookbook resolves to this worktree's src/. No TYPESAFE_API_KEY,
offline.

Check Result
ruff check . All checks passed
ruff format --check . (ruff 0.17.1, the pinned version; covers Python blocks in *.md) 397 files already formatted
pytest -q -p no:cacheprovider (Python 3.10.21) 5409 passed, 49 skipped, 50 deselected, 31 xfailed in 209 s, exit 0
python tools/check_hygiene.py Hygiene check passed: 606 file(s) scanned
python tools/check_recipe_assets.py Image-asset check passed: 461 file(s) under recipes/ checked
python tools/render_catalog.py --check exit 0
python tools/notebook_ci.py fixtures 51 recipe folders, 0 failed
tools/check_recipe_scope.py (foundation branch, body Closes #311) scope ok: foundation pull request: the recipe allowlist does not apply
git diff --raw -M origin/main...HEAD 8 files, all 100644 -> 100644 M (table above)
Notebook execution not applicable: no notebook or recipe references localization_* or gold_box
Guard demonstration (validator) fires on an inverted box, a non-Box sibling and an extra sibling key, on a real recipe copy; silent on nested labels, as documented
Guard demonstration (docs tests) both new docs-structure tests fail when the statements they pin are removed; worktree restored, head unchanged
Docs example executed outside pytest printed text matches the text block byte for byte

Environment note, reported rather than worked around: /workspace was completely full (7 MB free)
when I started, and one git diff lost its output to ENOSPC. I reclaimed
/home/agent/.cache/pip (531 MB, a recomputable download cache; not ~/.cache/uv) to get
working room, and removed no worktree, branch, venv or file belonging to another agent. My venv,
scratch scripts and worktree are removed at the end of this task. I did not exercise Python 3.14;
the PR body reports 5408 passed there on 865dfcc, and the only interpreter-sensitive code added
is plain arithmetic and math.fsum.

Acceptance criteria of #311, one by one

Criterion Status Evidence
Gold Box label format defined in docs/fixtures.md ({"present": true, "box": {top, bottom, left, right}} / {"present": false}) Met docs/fixtures.md:105-143, "Box labels": the exact shape #311 proposes, the strict-key rule, the [0, 1] / positive-size / no-underflow rules, "null is never a label", and the nesting and store-once rules
vision.gold_box(label) decodes it and validates geometry, with tests Met src/jev_cookbook/vision.py:320-349; tests/test_vision.py:894-947 (happy path including a JSON round trip, 26 parametrized rejections, a dedicated degenerate-gold test that also goes through the report)
The fixture validator rejects malformed Box labels when a recipe declares Box labels Met _check_box_labels at src/jev_cookbook/fixtures/__init__.py:556-581, wired at 722-725; tests/test_fixtures.py:338-400; I reproduced it on a copy of recipe 01 (table above). The declaration is implicit, via the reserved top-level present key; nested labels are out of its reach by design (F5)
localization_report(gold, outcomes, *, iou_threshold=0.5) taking ("located", box) / ("absent", None) / ("review", None) Met src/jev_cookbook/vision.py:672-742; exact signature, and the kinds are the tuple the documented localization_outcome policy already returns
...returns n_total Met field n_total; n_present + n_absent == n_total asserted by counted_once() in every report test
...a present x outcome table and an absent x outcome table, counting every example exactly once Met present_table / absent_table properties; each example increments exactly one cell (vision.py:715-721); both sums asserted against n_present / n_absent
...mean IoU over located-present and mean IoU over all present with missed/review counted as 0, each with its denominator named Met mean_iou_located / mean_iou_present; denominators gives present_located / n_present; __str__ prints "(denominator N: located present targets)" and "(denominator N: all present targets)"
...the hit rate at the threshold Met hits, hit_rate over n_present, inclusive >=, threshold validated to (0, 1]; test_report_hit_threshold_is_inclusive_and_configurable
...false positives on absent targets Met false_positives, false_positive_rate; a located box on absent gold never yields an IoU (test_box_returned_for_an_absent_target_is_a_false_positive_never_an_iou)
...correct absents Met correct_absents, correct_absent_rate; a review is never counted as one
A printable form that names every denominator Met LocalizationReport.__str__ (vision.py:648-670); test_report_prints_every_denominator_for_the_documented_example pins all eight lines; a zero denominator prints n/a (denominator 0: ...)
Tests show a reviewed present target changes the stated denominator rather than vanishing Met test_a_reviewed_present_target_changes_the_denominator_instead_of_vanishing (tests/test_vision.py:1045-1066)
The tests use hand-computed values Met dyadic rectangles with the derivation written out at tests/test_vision.py:876-881; I re-derived the documented example and a separate gold of my own
localization_metrics kept for compatibility, its docs pointing to the report Met behaviour unchanged (vision.py:513-550, body untouched); docstring now says "Kept for compatibility; prefer localization_report" and names the three gaps; docs/vision.md:262-285 "The older localization_metrics"; tests/test_vision.py:1285-1293 pins both the docstring pointer and that the old function still works
docs/vision.md and docs/evaluation.md updated; the worked example matches the test Met docs/vision.md:160-285 (gold labels, the report, the worked example, tiled recipes, the older function) and docs/evaluation.md:739-770 (the denominator table and the tiled paragraph); the example is executed and its stdout compared byte for byte by test_the_documented_localization_example_runs_and_prints_what_the_docs_show, and reproduced independently by test_report_prints_every_denominator_for_the_documented_example. I also ran it outside pytest

All five acceptance criteria met.

Prior reviews (read only after the above)

Comment 6306161479 (review 2 of 2 on head 34249bf, CHANGES-NEEDED), two blocking findings:

  1. "docs/fixtures.md:129-132: the nested-label example is contradictory, and rank 83 will copy
    it"
    - {"tile": "b3", "target": {"present": false}} named a gold tile for an absent target
    and stored a tile choice beside the box, which cannot hold the gold when overlapping tiles put
    a seam target in two tiles. Resolved at this head. The example is now
    {"id": "t12", "label": {"target": {"present": true, "box": {...}}, "rubric": 3}} - the exact
    replacement that review proposed, with no tile key. docs/fixtures.md:139-143 adds the
    requested sentence: store the gold once as a full-image Box label, store no tile choice, with
    both reasons given (an absent target has no right tile, only "no match"; overlapping tiles put a
    seam target in more than one tile), and Python derives the correct tiles from the box and the
    tile bounds. It is now mechanically pinned too:
    test_every_label_example_in_the_box_labels_docs_is_valid_and_consistent decodes every JSON
    example in the section through gold_box (recursing into nested objects) and rejects any tile
    key - and I confirmed by mutation that reintroducing the old example fails that test.

  2. "docs/vision.md does not say what one example is when a recipe tiles the image", with
    three specific statements requested. Resolved at this head. The new docs/vision.md:244-260
    "Tiled and hierarchical recipes" carries all three: the headline report is per image against the
    full-image gold, computed after tile_to_image; a per-tile report is a stage diagnostic that
    must be labelled as counting tiles and must state its rule for a target a tile edge cuts (named
    as the minimum visible fraction), with the reason that the rule changes the per-tile
    denominators; stage-1 "no match" maps to absent and an uncertain tile choice (or an uncertain
    Box in the chosen tile) to review. docs/evaluation.md:765-770 carries the same in short form
    with a link, and the section is reachable from docs/fixtures.md as well.
    test_the_docs_say_a_tiled_recipe_reports_per_image_not_per_tile pins all seven statements plus
    both cross-links, and I confirmed by mutation that it fails when the per-image statement goes.
    My F4 is a wording refinement inside this new text, not a reopening of the finding.

    Its non-blocking suggestions: "say explicitly that the test must cover every split" -> done
    (docs/fixtures.md:131-132, pinned by the docs test's "must cover every split" assertion). An
    image_to_tile helper -> not done, correctly out of scope (see my observations). An error
    message saying present is reserved -> not done (my F7). A single review total -> not
    done
    (my observations). The "ambiguous hard cases" sentence for "Box labels" -> not done;
    I did not independently judge it blocking either, since the strictness it describes (a correct
    review on an ambiguous example counts 0 in mean_iou_present and is not a hit) is visible in
    the report's own printed denominators.

Comment 6306168056 (review 1 of 2 on head 34249bf, APPROVE) - no blocking findings; its
eight mutation kills and its acceptance-criteria assessment agree with mine. Its three
suggestions: review_rate on the report -> not done (my observations; two reviewers have now
asked, which is the strongest case for a follow-up); a sentence saying a Box recipe that never
writes present is unchecked by the validator and is enforced instead at gold_box /
localization_report time -> not done, and F5 is the same gap seen from the other side;
_check_box_labels gated on inputs_ok -> not done (my F6, which that review also called
minor).

Nothing in either prior review is unresolved in a way that should hold this head: both blocking
findings are fixed and are now guarded by tests I verified fail without them. The suggestions that
remain open are ones I independently reached as well, and I do not consider any of them a merge
blocker.

Would I be comfortable with this merging as it stands?

Yes. It is a well-scoped foundation change that makes the thing #311 was opened about impossible: a
reviewed or absent Box case can no longer leave a denominator unnoticed, because every example is
counted once into a printed table whose denominators are named, and the one label format nine
recipes will copy is now specified, decoded by a single function and checked by the validator in
the flat case. The findings above are documentation precision (F1-F4), one design question worth
settling before the first tiled recipe but not before this merge (F5), three small polish items
(F6-F8), and one cross-reference (F9).

OPUS-REVIEW: APPROVE 23182da

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.

Foundation: Box evaluation can drop reviewed and absent cases from the IoU denominator, and there is no label format for an absent target

1 participant