Repository navigation
Foundation: Box evaluation report and gold Box label format (#311) - #331
Conversation
…r check (#311) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Opus reviewer (independent review 2 of 2): Claude Opus 5.5 ( Reviewed head The code is correct, and the mutation tests show its rules are enforced. Blocking
Suggestions (not blocking)
How each recipe would use the interfaceI wrote a sketch for this (run offline, with scripted answers, using this branch's
Checks run (fresh venv,
|
|
Opus reviewer (independent review 1 of 2): Claude Opus 5.5 ( Reviewed head Blocking findingsNone. Check against
|
| 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
ValueErrorwith the example index. np.bool_(True)forpresentis 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_labelscall 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
- A reserved top-level
presentkey declares Box labels. I accept this. A false trigger needs a non-Box recipe whose label object has a top-levelpresentkey, and the reservation is documented indocs/fixtures.md("Box labels");validate --allshows no current recipe uses one. A recipe that writes Box labels withoutpresentanywhere (bare boxes,{"absent": true}) passes the validator, but it fails loudly as soon as it callslocalization_report, which runsgold_boxon 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. - 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 pluslocalization_report([l["target"] for l in ...]), still enforces the format at scoring time. Walking arbitrary nested objects forpresentkeys would bring back the false-trigger risk at every depth. - 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.mdpoints toevaluate_outcomesfor the presence question's review rate. Strong suggestion below, because every Box README will quote this number.
Suggestions (non-blocking)
- Add
review_ratetoLocalizationReport. Define it as(present_review + absent_review) / n_total, add it todenominators, 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 usespresentis not checked by the validator, and that the format is enforced whenlocalization_reportorgold_boxruns. That sets the right expectation for the 83 and 98 builders. _check_box_labelsruns only when the inputs parse (fixtures/__init__.py, theif 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
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
PR #331 review — reviewer 1 of 2 (independent)
Diff reviewed ( Verdict summaryI found no correctness defect in Merge commit (
|
| 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)
src/jev_cookbook/vision.py:710, 724—guessleaks across loop iterations. It is
assigned only insideif 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 readguessoutside that guard would silently
score the previous example's rectangle instead of raisingUnboundLocalError. Compute
the IoU inside the located branch, or setguess = Noneat 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.src/jev_cookbook/vision.py:724—box_iouis called outside thetry, so a raise from
it would lose theexample N:prefix the docstring anddocs/vision.mdpromise. It is
unreachable today, precisely becausegold_boxguarantees a non-underflowing gold area,
but moving the call inside the guarded block costs nothing and keeps the promise true
independently of that invariant.src/jev_cookbook/vision.py:694, 741—hitscompares against the uncoerced
iou_thresholdwhile the report storesfloat(iou_threshold). Any non-floatReal
(afractions.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.src/jev_cookbook/fixtures/__init__.py:573-579— the misfire message does not mention
the reservation. The declaration mechanism is "a top-levelpresentkey", which
docs/fixtures.mddocuments 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 atdocs/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 onmainis affected, and every failure it can cause is loud.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 apresentkey — exactly what the validator declines to do. Consider having
_check_box_labelsdo 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
trainsplit in particular never passes throughlocalization_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 reservepresentat every depth.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.jsonland 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 toif labels_ok:would report both in one run.src/jev_cookbook/fixtures/__init__.py:569— the deferred-import comment's stated reason
does not hold. "deferred: validating other recipes needs no Pillow" —Pillowis 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.docs/vision.md:244-247— the error sentence overreaches. "Malformed gold, an unknown
outcome kind, alocatedoutcome without a valid box, a box attached toabsentor
review, empty or unequal-length inputs and aniou_thresholdoutside (0, 1] raise
ValueErrornaming 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.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 messagef"outcome kind must be one of {_OUTCOMES}"points at a name a reader cannot
import. Exporting the tuple in__all__would make both referenceable.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 callsgold_box(label["target"])in
its own test, butlocalization_reporttakes labels, not decoded boxes, so the reporting
call islocalization_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, whichgold_boxrejects with "a gold Box label must be an object".- No
n_review/review_rateon the report.present_review + absent_reviewis 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:762does route the reader toevaluate_outcomesfor 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
- Gold Box label format in
docs/fixtures.md, plusvision.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 intest_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-levelpresentkey, and a nested Box label is not validated. localization_report(gold, outcomes, *, iou_threshold=0.5)taking("located", box)/
("absent", None)/("review", None), returningn_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-752andLocalizationReport(:567-670):present_table,absent_table,
mean_iou_located,mean_iou_present,hit_rate,false_positives,correct_absents,
absent_review,denominators.- 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_vanishingholds gold
fixed and assertsmean_iou_located1.0 at denominator 1 againstmean_iou_present0.5 at
denominator 2, then asserts both strings appear instr(report). The comment block at
tests/test_vision.py:875-883derives every IoU by hand from the four rectangles. My M1
and M8 mutations confirm those tests fail when a review vanishes. localization_metricskept 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_remainsasserts both the
docstring pointer and that the old function still computes.docs/vision.md:269-274keeps
it as "The olderlocalization_metrics".docs/vision.mdanddocs/evaluation.mdupdated; the worked example matches the test. —
Met.docs/vision.md:157-247(gold labels, the report, the worked example, the tiled
section) anddocs/evaluation.md:739-769(the denominator table and the tiled paragraph).
test_the_documented_localization_example_runs_and_prints_what_the_docs_showextracts the
fenced block fromdocs/vision.md,execs it and compares stdout with thetextfence
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:
- "
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.865dfccreplaces 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_consistentparses every JSON
example in the section, runsgold_boxon every nested object with apresentkey, asserts
no example contains any key matchingtile, 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. - "
docs/vision.mddoes not say what one example is when a recipe tiles the image" —
Resolved at this head.865dfccadds### 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 aftertile_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 adocs/evaluation.mdrow 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 indocs/vision.mdplus thedocs/evaluation.mdsentence and the cross-link from
bothevaluation.mdandfixtures.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)
/workspacewas full when I started (0 MB free; tool output was being lost toENOSPC),
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-tmpstill holds ~1.8 GB of other stale session scratch and
~/.cache/ms-playwright1.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:525says 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
PR #331 review (independent review 2 of 2)
I formed the view below from the diff, issue #311, Verdict summaryThe code is correct, the tests are strong and genuinely hand-computed, the new validator check Must changeNone. Findings (all non-blocking, ordered by severity)F1.
|
| 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_consistentFAILED
withValueError: 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_tileFAILED 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, theBoxrow ("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 alocatedoutcome; 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 printingn/a, never 0.- Ranks 77, 78, 79, 81, 82 (
Box+Noul): the natural label is{"present": ..., "box": ...}, presence gold isgold_box(label) is not None, and the documentedlocalization_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 ownlocalization_reportagainst the same gold, andioushas 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 aftertile_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, butn_totalbecomes a tile count andcorrect_absent_ratemostly measures empty tiles -
exactly what the docs now forbid quoting as the recipe's result. - Absent targets:
{"present": false}is a first-class label,nullstays forbidden by
schema.json, and a located box on absent gold is a false positive withious[i] is None. - Uncertain / review: never an absent prediction and never a correct absent
(test_report_all_review_keeps_every_example_in_the_denominatorasserts
correct_absents == false_positives == present_missed == 0with 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 isdocs/vision.md:120-127, and its new clause is true -gold_boxrejects a
degenerate gold label up front, sobox_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:
-
"
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 notilekey.docs/fixtures.md:139-143adds 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_consistentdecodes every JSON
example in the section throughgold_box(recursing into nested objects) and rejects anytile
key - and I confirmed by mutation that reintroducing the old example fails that test. -
"
docs/vision.mddoes not say what one example is when a recipe tiles the image", with
three specific statements requested. Resolved at this head. The newdocs/vision.md:244-260
"Tiled and hierarchical recipes" carries all three: the headline report is per image against the
full-image gold, computed aftertile_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 toabsentand an uncertain tile choice (or an uncertain
Box in the chosen tile) toreview.docs/evaluation.md:765-770carries the same in short form
with a link, and the section is reachable fromdocs/fixtures.mdas well.
test_the_docs_say_a_tiled_recipe_reports_per_image_not_per_tilepins 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_tilehelper -> not done, correctly out of scope (see my observations). An error
message sayingpresentis 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 inmean_iou_presentand 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
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, booleanpresent, noboxon an absent label, edges in [0, 1], positive size, and a gold area that does not underflow to zero, sobox_ioucan 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)andLocalizationReport: 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)andreport.denominatorsname every denominator. A box returned for an absent target is a false positive, never an IoU; a review is never an absent prediction.presentkey declares Box labels; each must passgold_box, and a non-Box label among them is an error.schema.jsondescription points to the format.localization_metricskept; its docstring anddocs/vision.mdpoint to the report.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 callgold_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-levelpresentkey 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.vision.localization_*, so no notebook execution is affected.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.