Repository navigation
Foundation: image preparation, tile inverse, multi-level box mapping and a 280 px minimum check (#310) - #341
Conversation
…and a 280 px minimum check (#310) Adds prepare_image (resize/pad to the Images API limits, returning an ImageTransform), ImageTransform (crop/then/to_original/to_derived/visible_fraction, exact rational arithmetic), image_to_tile, crop_bounds, and an opt-in min_side for generate_tiles and crop_box. Documents the helpers with a runnable tile-choose-crop-prepare-Box-map-back example. Refs #310 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Review of PR #341 — reviewer 2 of 2 (independent)
Verdict in one paragraphThe implementation is correct as far as I can test it, the API is a good fit for the recipes that Must changeM1. No test pins
|
| Check | Result |
|---|---|
ruff check . |
All checks passed!, exit 0 |
ruff format --check . |
405 files already formatted, exit 0 (the body's count is right) |
pytest (full) |
6305 passed, 55 skipped, 50 deselected, 31 xfailed in 250.74s, exit 0 — identical to the body's numbers, and to CI's Tests (py3.10) row (the body labels this a 3.14 venv; with [dev] only, tests/test_live_sdk.py is skipped, so 3.14 gives the same counts — CI's .[dev,live] 3.14 job reports 6368/53) |
pytest tests/test_vision.py ×2 |
389 passed in 4.62s then 389 passed in 3.10s — same count, no flakiness, fast |
python tools/check_hygiene.py |
Hygiene check passed: 615 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 |
exit 0 |
| Worked example, run outside its test | prints the documented five lines, MATCH: True |
12 independent probes (size table, forgotten min_side, error texts, exactness of a 3-level chain, degenerate inputs, ULP sliver, identity case) |
as reported above |
| 9 hand-made mutations of the new code | 8 caught (strict_min, no_snap, pad_odd_left, round_half_down, no_tile_check, touch_overlaps, upscale_unbounded, no_crop_check); 1 survived (then_wrong_window, M1) |
| Commit hygiene | head is signed (G), one commit on ebf2413, Refs #310 in the message, Co-Authored-By: Claude Sonnet 5.5 present |
| CI on this exact head | all rows pass, including Tests (py3.10) 3m16s and Tests (py3.14) 5m11s (read, not assumed) |
No notebook executes any of this code (no recipe uses the new helpers yet), so there was nothing to
execute from a scratch copy; Notebooks (execute) is green on the head and Fixtures (validate)
passes. No network call, no API key, no live inference.
Scope
git diff --raw -M origin/main...HEAD:
:100644 100644 57990b5 6bcd020 M docs/vision.md
:100644 100644 959bfe9 a813094 M src/jev_cookbook/vision.py
:100644 100644 fe139f1 7b1abbe M tests/test_vision.py
Three files, exactly the three #310 names, all 100644 → 100644 modifications: no symlink (120000),
no submodule (160000), no mode or type change, no rename, no deletion. +1324/−8. This is
foundation work on a foundation/ branch, so the recipe allowlist does not apply (and Scope (recipe pull requests) passes). Nothing in catalog/, tools/, .github/ or recipes/. No
secrets, no absolute home paths, no TYPESAFE_API_KEY reference in the diff.
PR body against the diff
Accurate, with the two qualifications already noted. Verified claim by claim: the three files; the
prepare_image behaviour and signature; ImageTransform's members and the exact-Fraction,
round-once arithmetic; the pixel-edge snapping; the "too small for a float rectangle" raise;
crop_bounds being what crop_box cuts (crop_box does now call it); the opt-in min_side on both
functions raising before any crop; the three new doc sections and the executed example; the removal
of the "Padding or further cropping requires its own transform" sentence; tile_to_image changed only
in its docstring (the diff's only deletions are the old crop_box body/docstring and that one
docstring line). The ruff and pytest counts are exactly what I measured. Decision 8 checks out: I
grepped origin/foundation/vision-utilities, multimodal-api, multimodal-base and
multimodal-integration for prepare_image|pad_color|image_to_tile under src and docs and found
nothing, so there was nothing to reuse. Closes #310 is the only closing keyword anywhere in the
body (the only other # reference is the title's (#310)), and the recipe checklist is correctly
omitted — the template says to skip it for foundation work. The two qualifications: the red-check
bullet overstates coverage of the then window-remap branch (M1), and "docs/evaluation.md line 766
… correct as it stands" is better described as incomplete for multi-level recipes (S1).
Acceptance criteria of #310
| Criterion | Verdict |
|---|---|
prepare_image(image, *, min_side=280, max_side=1568, pad_color=...) returns (prepared, transform) mapping normalized boxes prepared→original and back; round-trip tests at several aspect ratios including padding on one axis |
Met. Signature matches (plus upscale). test_boxes_round_trip_through_prepare_image_at_every_aspect_ratio covers 11 sizes × upscale ∈ {False, True} in both directions, and test_prepared_images_meet_the_api_limits_for_every_aspect_ratio re-validates each result through validate_image_asset. Pad-one-axis is (300, 200)/(200, 300), checked pixel by pixel in test_prepare_image_pads_one_axis_and_keeps_every_pixel. |
image_to_tile(box, tile_bounds, width, height) returns the clipped tile-local box or None; tested |
Met. Exact signature; hand-computed inside/clipped/covering/disjoint/edge-touching cases, plus an inverse check against tile_to_image over every generated tile and invalid-bounds/invalid-box rejection. |
| Multi-level mapping documented and tested; a box two levels down maps to the full image exactly | Met. test_a_box_found_two_levels_down_maps_to_the_full_image_exactly asserts ==, not approx; a three-level chain with a pad in the middle, a pixel-verified chain on a real image, associativity, and resize→crop / crop→resize+pad orders. Documented in "Mapping boxes between an image and what you sent". M1 is a gap within this criterion's testing, not a failure of it. |
generate_tiles and crop_box can be asked to enforce min_side (or prepare_image documented as required), with a test that a sub-280 tile is refused or padded |
Met. min_side= on both, checked before any crop (proved by monkeypatching Image.Image.crop to fail), exactly 280 accepted, 279 refused, expanded crop measured after expand, message naming the size and prepare_image, invalid min_side rejected. The documented requirement is also stated in bold at docs/vision.md:338-339. |
docs/vision.md worked example: tile → choose → crop → prepare → Box → map back |
Met. All six stages, runnable offline, output compared by a test and by me independently. |
Outside my task (reported, not fixed)
origin/recipe/83-hierarchical-visual-search'shelpers.py:79enforcesmin(size) < 256and a
78,400 area floor by hand — the 256 is the pre-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 minimum and is now wrong. When that branch is
integrated it should becomemin_side=IMAGE_LIMITS["min_side"]ongenerate_tiles. Same file
builds request assets by hand (asset_for) rather than throughimage_asset; it happens to stay
safe becauseimage_referencevalidates, but it is worth checking during that recipe's review.- There is still no documented way to turn a runtime
PIL.Image(a tile, a prepared crop) into a
request asset:image_assettakes a path only, anddocs/vision.md:378-379says to "save the
prepared image and load it withimage_asset" without saying where a notebook may write (the
local-CI instructions indocs/development.md:119-134comparegit status --porcelainbefore and
after, so it cannot be the checkout). Every tiling recipe will need this; it deserves its own
foundation issue rather than a line in this PR. validate_image_asset's under-280 message (image width and height must each be 280 to 1568 pixels, from 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: enforce the Images API's 280 px minimum, hash image bytes in replay keys, bound tiles (#306) #321) does not point atprepare_imagethe way the newmin_sidemessage does.
A builder who forgetsmin_sidemeets the less helpful of the two.
OPUS-REVIEW: CHANGES-NEEDED 308cac8
Review of PR #341 — reviewer 1 of 2 (independent)
Verdict in one paragraphThe coordinate maths is right. I re-derived the whole model independently in exact rationals and Must changeM1 —
|
| path | prepared.info keys |
|---|---|
| already fits (no resize, no pad) | dpi, exif, icc_profile |
| resize only | icc_profile |
| pad (fresh canvas) | (empty) |
Saving the first two as PNG writes an iCCP chunk. Driving the repository's own checker
(tools/check_recipe_assets.check_image) on those files:
no-pad path (400 x 300 already fits): [png-metadata] chunk iCCP is not allowed ...
resize path (2000 x 1500 -> 1568 x 1176): [png-metadata] chunk iCCP is not allowed ...
pad path (300 x 200 -> 300 x 280): []
docs/fixtures.md's Image.frombytes copy: []
Why this matters rather than being a Pillow footnote:
docs/fixtures.md:361-363already names this exact hazard in the project's own words — "saving
to PNG does carry a source's ICC profile across (theiCCPchunk fails the check), so do not
re-save the opened file itself" — and prescribes theImage.frombytes("RGB", rgb.size, rgb.tobytes())clean copy.prepare_image's no-op path is effectively re-saving the opened
file.- The new
docs/vision.mdsection points the reader the other way: "prepare_imagedoes not
certify the result for the API: save the prepared image and load it withimage_asset, which
re-checks format, size and the file limit."image_asset/validate_image_assetdo not
check PNG chunks (I confirmed: theiCCPfile is accepted), so a recipe builder who follows
onlyvision.mdgets a green local validation and a redHygienejob. - Because the pad path is clean, the failure is shape-dependent: the same code produces a
compliant asset for a small crop and a non-compliant one for an image that already fits. That
is the hardest kind of rule to learn from, and ranks 63–84 and 98 copy this interface.
What to change — one line plus one sentence:
- build the result from raw pixel bytes so all three paths behave alike, e.g. replace the final
preparedwithImage.frombytes("RGB", prepared.size, prepared.tobytes())(or clear
prepared.info), and say so in the docstring ("the result carries no metadata from the
source"); - in
docs/vision.md's "Preparing an image for the API", add that the prepared image carries no
metadata, pointing at the image-file contract infixtures.md; - add a test: save a
prepare_imageresult whose source hasinfo["icc_profile"]and assert the
PNG's chunk list isIHDR/IDAT/IEND(or has noiCCP), for the fits, resize and pad paths.
Without such a test this regresses silently — nothing in the 6,305-test suite touches.info.
This is the only item I would hold the merge for.
Suggestions (not blocking)
- S1 — nothing pins the resampling filter.
src/jev_cookbook/vision.py:800. My mutant
replacingImage.Resampling.LANCZOSwithNEARESTwas the only one of 27 to survive the
whole oftests/test_vision.py(389 passed). The docstring anddocs/vision.mdboth state
"resampling is Lanczos", and the filter is what decides the bytes of every generated asset, so
a silent change would move committed asset digests and the replay keys computed from them. A
cheap guard that does not pin Pillow's exact implementation: downscale a hard black/white edge
by a non-integer ratio and assert at least one intermediate grey value appears (NEARESTand
other point filters cannot produce one). - S2 —
upscaleis unvalidated while every neighbouring argument is strict.
vision.py:745—prepare_image(..., upscale="yes")is accepted and treated as true, and
upscale=0/upscale=Noneas false, whereasmin_side=280.0,min_side=Trueand
pad_color=Trueare all rejected by name. Aif not isinstance(upscale, bool): raisewould
match the module's own standard. - S3 — EXIF orientation is not mentioned.
prepare_image's docstring covers transparency and
pixel modes but not orientation, and the no-op path carriesinfo["exif"]through.
_image_formatrejects an asset whose orientation tag is not 1, anddocs/fixtures.md:376
tells authors to runImageOps.exif_transposebefore the copy. One sentence in the docstring
("rotate orientation into pixels withImageOps.exif_transposefirst") closes the loop. (No
PNGeXIfchunk is written in practice on either Pillow version I tested, so this is
documentation, not a defect.) - S4 — four added lines exceed the configured
line-length = 100:vision.py:371(108),
:380(102),:597(114), plusdocs/vision.md:307(129) and two 103–104-char test lines.
ruff checkis silent becauseE501is not in theselectlist, andruff formatcannot
reflow a docstring, so nothing flags them;origin/mainhas exactly one such line in this file
and none intests/test_vision.py. Worth reflowing for consistency, not worth a round trip on
its own. - S5 — the "too small to represent as a float rectangle"
ValueErrorescapes the
None-returning API (vision.py:563, reached fromto_derived,to_originaland
image_to_tile). I could not reach it from anything realistic: 47,486image_to_tilecalls
over random realistic tilings (five page sizes, five tile widths, gold boxes written as
decimals, as pixel ratios, and as the output oftile_to_image) produced zero raises, and
200,000prepare_image-shaped pad mappings produced zero. It only appeared in my generator's
deliberately pathological pads (content of 1–39 px on a canvas up to 300 px larger), 128 times
in 40,000 chains. So I agree with the design, but a recipe that loopsimage_to_tileover
every tile to build per-tile gold has one code path that raises instead of skipping; a sentence
inimage_to_tile's docstring saying so (or in the docs bullet that already mentions the
raise) would be kind to the eleven recipes that copy this. - S6 —
ImageTransform's public fields are annotated with the private_Edgesalias
(vision.py:603-604). The class is exported and its constructor is documented as usable
("the constructor only checks that the sizes and windows are valid"), but a caller who wants to
build one directly has no public name for the window type. Exporting the alias (or spelling it
tuple[Fraction, Fraction, Fraction, Fraction]) would be tidier. - S7 —
docs/evaluation.md:766still describes the headline outcome as "the final rectangle
aftertile_to_image", which is now only the single-crop case. The PR body discloses this and
it is not wrong as written; broadening it to "after mapping back (tile_to_image, or an
ImageTransformchain)" would be a one-line follow-up.
What I verified myself, and how
The coordinate maths
I loaded the geometry half of vision.py verbatim (PIL and sibling imports stubbed) and wrote an
independent exact-rational model of the whole pipeline from scratch — each step modelled as an
affine map between two windows, composed step by step with clipping at every level — then
compared it with the code's compose-then-clip-once implementation.
| property | cases | result |
|---|---|---|
random chains of 1–3 crop / resize+pad steps built with then |
37,362 | all built; the only ValueError was the documented "shows none of the first transform's content" |
to_derived against my exact model (float equality, not approx) |
20,338 | 0 mismatches |
to_original against my exact model (float equality) |
24,941 | 0 mismatches |
to_derived returning None where my model says empty |
14,126 | 0 disagreements |
then associativity: left fold == right fold |
37,362 | 0 differences |
visible_fraction against exact area arithmetic |
single-step subset | 0 differences |
So "a multi-level mapping is the true position rounded once" is not a claim I had to take on
trust: the composite result equals the exact rational position rounded once, to the last bit, in
every case I generated.
The snapping rule (_snap, vision.py:519-527)
- Does it ever snap something that is not a pixel edge? No. 500,000 random coordinates
against ten dimensions: every snap satisfiedvalue == k / dimension, and no snap moved a
coordinate by more than 1e-12 of a pixel (in fact ~1e-16). - Is it consistent with the pre-existing
_pixel_edgerule? Yes. For everyk/dimension
withdimensionin 1–399 plus 512, 777, 1000, 1024, 1568, 2048, 4000, 10000 and 99991,_snap
snapping and_pixel_edgesnapping agreed: 0 disagreements. (_snapdrops_pixel_edge's
extraabs(scaled - nearest) <= ulpguard, which the equality test implies in practice.) - Can it hide a real sliver? No. Pushing a box edge past a tile edge at four
(width, pixel)combinations plus a 3-px image:exactly k/W -> None, and one float step
past -> a sliver is reported (3.9e-14 px at W=1000), as are 1e-15, 1e-12, 1e-9, 1/1000 px,
1/10 px and one whole pixel past. A genuinely 1e-9-wide box wholly inside a tile is returned
unchanged. The same holds at aprepare_imagepad boundary: exactly the content edge gives
None, one float step past gives a clipped box. So the rule collapses only the exactly
representable pixel edge, which is the documented intent.
Consistency with the existing semantics and the 280 px rules
image_to_tile/tile_to_imageround trip: 20,000 random(image size, tile bounds, tile-local box)triples — 0 failures, 0 raises, max error <= 1e-9, andNonenever returned
for a box the forward map produced.ImageTransform.crop(...).to_originalagrees withtile_to_imagefor generated tiles (the
PR's own test, which I confirmed is a genuine new-vs-old cross-check, not a tautology).generate_tiles's docstring claim that themin_sidecheck is exact — "every tile has the
same size,min(tile_width, width)bymin(tile_height, height)" — is true:starts()
only returns positionspwithp + window <= length, somin(left + tile_width, width)is
never a partial tile. The check is therefore neither conservative nor permissive.prepare_image's output is always inside the limits by construction (content <= max_side
from the clamp inscaled,size = max(content, min_side)), which also puts the area inside
78,400–2,458,624 at the defaults. The PR's parametrised test proves it end to end by saving a
PNG and runningvalidate_image_asset.IMAGE_LIMITS(280/1568/78,400/2,458,624) matchestools/check_recipe_assets.py:48-51
exactly.- Hand-recomputed the whole
prepare_imagedocs table independently (all seven rows and all five
content rectangles): correct, including1000 x 1568/1601 = 979.39 -> 979, the odd-pixel row
201 x 100 -> columns 39..240 (39 left, 40 right)and4000 x 100 -> rows 120..159. - Hand-recomputed the worked example's three-level chain in exact fractions:
to_originalgives
7/10, 11/30, 4/5, 13/30, whose floats are bit-identical to840/1200, 330/900, 960/1200, 390/900, so the example'sfound == goldandbox_iou == 1.0are true for the right reason,
andnormalized_to_pixelsrecovers(840, 330, 960, 390)only because of the pre-existing
_pixel_edgesnapping (without it the right edge would ceil to 961).
prepare_image vs the image-file contract and replay keys
- Does it change bytes? Yes by design (it converts, resamples and/or pads); the input is not
modified, which the PR's test proves by mutating the output. - Deterministic across Pillow versions? Yes, for the two versions this repository's CI uses.
I hashedprepared.tobytes()for twelve(size, upscale)combinations on a textured synthetic
page and the saved PNG bytes for two of them, under Pillow 13.0.0 / Python 3.14.7 and
Pillow 12.3.0 / Python 3.10.21: every pixel digest and both PNG file digests were
identical. Note also that the common path for a small crop (upscale=False) performs no
resampling at all — it is a paste onto a fresh canvas — so it is trivially reproducible. The
residual risk is a future Pillow changing its Lanczos kernel, which only S1 would catch. - Replay keys:
replay_keyhashes decoded file bytes of assets
(src/jev_cookbook/backends.py:184-193), so a recipe that prepares an image at notebook run
time and sends it is only as reproducible as the prepared bytes; given the measurement above
that is currently fine, and the pad-only path needs no assumption at all. Nothing in the diff
changesreplay_keyorimage_reference. - The contract interaction is M1 above.
Degenerate, extreme and non-numeric inputs (all with the real module)
NaN, ±inf, negative, >1, zero-width, zero-height, three-edge and non-mapping boxes all raise
ValueError with a named edge, from to_derived, to_original and visible_fraction alike.
Non-integer tile bounds, float width, min_side=nan, min_side=0/-280/True/280.0/"280" and
every bad pad_color in the PR's table raise. prepare_image on 1 x 1 -> 280 x 280 with window
139/280..140/280; 1 x 1 with upscale=True -> 280 x 280; 10000 x 1 -> 1568 x 280; 279 x 279 ->
280 x 280; 1568 x 1569 -> 1567 x 1568 (aspect kept, inside the limits). A 1-px image tiles and
maps fine. A hand-built transform with a 10**9 nominal size works; 10**400 raises
OverflowError rather than ValueError, which is unreachable from any real image and not worth
changing. to_derived accepts a Box-answer object and a {"box": ...} mapping, like the rest of
the module.
Are the tests real checks? My own mutants
I wrote 27 mutations of vision.py of my own (not the PR's nine) and ran
pytest tests/test_vision.py -x -q -p no:cacheprovider against each. 26 were killed, the
survivor being S1 (LANCZOS -> NEAREST):
killed — never snap a pixel edge; always snap; _clip counts touching as overlap; _remap swaps
fx/fy; scaled uses ceil / round-half-down / floor; the odd pad pixel goes left/top; content in
the top-left instead of centred; min_side strict (<=); min_side checks the long side;
generate_tiles ignores a source smaller than the tile; generate_tiles skips the check;
crop_box skips the check; crop_box checks after cropping; then uses the wrong window in the
first _remap; then drops the empty-intersection check; upscale ignores max_side;
upscale overrides a downscale; _as_box returns a collapsed box; to_original clips to the
wrong window; visible_fraction returns an area not a share; no min_side > max_side check;
pad_color accepts 256; transparency and odd modes allowed through; crop_bounds drops the
expansion.
The worktree was restored afterwards (git status clean, git diff --stat empty).
On tautology: I found none. The expectations are hand-computed integers and fractions in comments
beside each assertion; the pixel-level tests (crop((0, 28, 300, 228)).tobytes() == image.tobytes(), the blue-rectangle getbbox() tests, crop_box(prepared, mapped).tobytes() == crop_box(image, box).tobytes()) are independent of the mapping code; and the two places that do
use the code under test to form an expectation —
test_image_to_tile_inverts_tile_to_image_for_every_generated_tile branching on
visible_fraction, and test_boxes_round_trip_... bounding the result with
transform.derived_window — both still carry an independent assertion, and the windows
themselves are pinned by hand elsewhere (window_of(transform) == (Fraction(0), Fraction(7, 64), ...)). test_generate_tiles_min_side_accepts_...'s first assertion compares the function to
itself, but the next two lines pin the tile size and the count (12) by hand.
Checks I ran
Fresh venvs under /home/agent/r1/ (uv venv --python 3.14 and --python 3.10, then
uv pip install --link-mode symlink -r a one-line requirements file holding
-e <worktree>[dev]), each heavy command under flock .claude/heavy.lock nice -n 10, pytest -p no:cacheprovider, no nested flock, available memory checked first (4.0–4.2 GB).
| command | result |
|---|---|
ruff check . (0.17.1) |
All checks passed!, exit 0 |
ruff format --check . |
405 files already formatted, exit 0 — matches the PR body |
pytest (3.14.7, Pillow 13.0.0) |
6305 passed, 55 skipped, 50 deselected, 31 xfailed in 257 s, exit 0 — matches the PR body exactly |
pytest tests/test_vision.py (3.10.21, Pillow 12.3.0) |
389 passed in 4.5 s — matches the PR body exactly |
python tools/check_hygiene.py |
passed: 615 file(s) scanned, exit 0 |
python tools/check_recipe_assets.py |
passed: 461 file(s) under recipes/ checked, exit 0 |
python tools/render_catalog.py --check |
exit 0 |
python tools/notebook_ci.py fixtures |
51 recipe folders, 0 failed, exit 0 |
| my independent exact-model property tests | 37,362 chains / 45,279 mappings, 0 problems |
my 27 mutants of vision.py |
26 killed, 1 survived (S1) |
prepare_image byte digests, Pillow 13.0.0 vs 12.3.0 |
identical for all 12 cases and both PNG files |
tools/check_recipe_assets.check_image on prepare_image output |
png-metadata (iCCP) on 2 of 3 paths — M1 |
the new docs/vision.md min_side code block, executed by hand |
ran clean |
No notebooks, recipes or fixtures are touched by the diff (docs/vision.md,
src/jev_cookbook/vision.py, tests/test_vision.py only), so no notebook execution was needed
and tools/check_recipe_scope.py has nothing to judge.
PR body against the diff
Accurate throughout; I checked every factual claim and every number.
- Signatures, defaults, step order, the "RGB with both sides in
[min_side, max_side]"
guarantee, the half-up rounding, the Lanczos filter, the odd-pixel-right/bottom centring, the
transparency/mode refusal, the "rounds to float once" claim, the snapping rule stated as "the
float nearestk / widthis read as pixelk" (which is exactlyvalue == float(k)/float(w)),
the "too small for a float rectangle raises" behaviour,image_to_tileas the inverse,
crop_boundsbeing whatcrop_boxnow uses, the opt-inmin_sideraising before any crop,
and "tile_to_imageis unchanged apart from its docstring" — all confirmed against the diff. - The old sentence "Padding or further cropping requires its own transform" is indeed gone, and a
test asserts its absence. - "How it was verified": all four reproducible numbers (405 files; 6305/55/50/31; four tools at
exit 0; 389 on 3.10) came out identical for me. I cannot replay the author's nine mutants, but
all nine shapes they describe are among my 27 and all nine were killed in my run. - Reuse claim: I fetched
foundation/vision-utilities,foundation/multimodal-apiand
foundation/multimodal-integrationand grepped theirsrc/andtools/for
prepare_image|ImageTransform|image_to_tile|def .*resize|LANCZOS— no hits, so "nothing was
reused" is correct. (foundation/multimodal-baseno longer exists on the remote; harmless
staleness in the body.) - "No other file was touched" — correct, three files, +1324/-8.
- No overclaim found. The one thing the body does not mention is M1.
The decisions offered to reviewers all seem right to me: opt-in min_side (changing the default
would force edits to existing tests, and the geometry functions legitimately run on tiny images —
and generate_tiles's check is exact, so turning it on later is safe); refusing rather than
padding in crop_box/generate_tiles; upscale=False; one transform type rather than two; and
the edge-snapping rule. On decision 1 I would answer: yes, revisit the default in a follow-up
once a recipe exists to try it against, not now.
Acceptance criteria of #310
| # | Criterion | Status |
|---|---|---|
| 1 | vision.prepare_image(image, *, min_side=280, max_side=1568, pad_color=...) returns (prepared_image, transform) mapping prepared -> original and back; round-trip tests at several aspect ratios, including padding on one axis |
Met. Exact signature; test_boxes_round_trip_through_prepare_image_at_every_aspect_ratio runs 11 sizes x upscale x 5 boxes, covering pad-one-axis (300 x 200, 200 x 300), pad-both (200 x 100), scale-then-pad (4000 x 100), scale-only and no-op. My own round trip over the same shapes agreed with exact arithmetic. |
| 2 | vision.image_to_tile(box, tile_bounds, width, height) returns the clipped tile-local box or None when it does not intersect; tested |
Met. Inside, clipped, covering, four directions of miss, edge-touching, one-pixel overlap, invalid bounds and invalid boxes are all tested; I verified the inverse relationship over 20,000 random tilings. |
| 3 | Multi-level mapping documented and tested: a box found two levels down maps to the full image exactly | Met. docs/vision.md has a table of steps, the chaining rules and the executed worked example; test_a_box_found_two_levels_down_maps_to_the_full_image_exactly and test_three_levels_with_a_pad_in_the_middle_... assert ==, not approx, and test_chained_transforms_agree_with_pixels_on_real_images ties it to real pixels. |
| 4 | generate_tiles and crop_box can be asked to enforce min_side (or prepare_image is documented as the required step), with a test that a sub-280 tile is refused or padded |
Met. Both take min_side=; both refuse before any crop (proved by monkeypatching Image.Image.crop to fail); exactly 280 passes, 279 fails with a message naming the size and prepare_image; and docs/vision.md states in bold that a recipe sending tiles or crops passes min_side=IMAGE_LIMITS["min_side"] or runs them through prepare_image. |
| 5 | docs/vision.md worked example: tile -> choose -> crop -> prepare -> Box -> map back |
Met. All six stages, offline, with its printed output compared by a test; I recomputed the whole chain by hand and it is exact. |
All five are met. M1 is about how the prepared image is saved, which #310 does not ask for but
docs/fixtures.md does.
Outside my task (reported, not fixed)
docs/evaluation.md:766(S7) — pre-existing wording, now slightly narrow.tests/test_docs_examples.pyonly indexesdocs/evaluation.md, sodocs/vision.md's code
blocks are executed only by the two bespoke tests intest_vision.py(the new worked example,
and the pre-existing frame walkthrough). The new "Minimum size" block is not executed by any
test; I ran it by hand and it is clean. Widening that harness is a separate issue.foundation/multimodal-baseis gone from the remote, so the PR body's branch list is stale by
one name.
OPUS-REVIEW: CHANGES-NEEDED 308cac8
…adata from prepare_image, document reporting and upscale (#310) Review fixes for the image-transforms PR: assertions that pin then()'s second remap, a clean metadata-free copy on every prepare_image path (checked against tools/check_recipe_assets.py), a ValueError for a zero-sized image, a resample-filter check, and docs on reporting a mapped-back None and on choosing upscale. Refs #310 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Re-review of PR #341 (round 2) — reviewer 2 of 2 (independent)
Verdict in one paragraphMy blocker is properly fixed: I replayed my own mutant ( Must changeM1. New prose makes an unmeasured claim about Jev, and contradicts the sourced paragraph above it
Two problems with the bolded clause:
What to change — the geometry is the real argument and needs no claim about the model, e.g.
Deleting the four words outright is equally fine. The figure itself is correct and I verified it Should change (not blocking)S1 (new). Stripping metadata also strips the orientation that
|
| Round-1 finding | Status |
|---|---|
M1 then()'s derived-window remap unpinned |
Resolved. tests/test_vision.py:1983-1992 adds the three assertions (derived_window == (1/7, 9/10, 6/7, 1); the padding-only top half → None; a partly-clipped box → nb(0.5, 0.0, 1.0, 0.05)), each with the arithmetic worked out in a comment. My original mutant now fails on the first of them, and a new variant that drops the original_window remap instead fails test_a_box_found_two_levels_down_maps_to_the_full_image_exactly. I also read the three values off the live objects: they are what the test says. |
S1 reporting guidance named only tile_to_image |
Resolved, in both places. docs/vision.md:253-262 and docs/evaluation.md:765-770 now say "after tile_to_image, or, when the recipe also cropped or padded inside the tile, after ImageTransform.to_original on the chain that .then built from every step", with a link to the mapping section. |
S2 no outcome named for a None from to_original |
Resolved and consistent (see below). |
S3 no guidance on choosing upscale |
Addressed, and the new paragraph is the right shape — except for the clause in M1. |
Suggestion 1, ZeroDivisionError on a zero-sized image |
Resolved. src/jev_cookbook/vision.py:785-786 adds the same _positive_int pair generate_tiles uses; I verified all six combinations of (0,0), (300,0), (0,300) × upscale raise ValueError naming the right axis, and removing the guard turns the file red. |
Suggestions 2, 3, 4, 5, 6, 7, 8 (message wording, tile bounds label, from_crop naming, crop+transform ergonomics, ULP sliver, long lines, docs rationale) |
Not taken, all explicitly non-blocking; I do not press any of them. The only new long line is the 104-character anchor link at docs/vision.md:258. |
Is the None → review guidance consistent with localization_report and the tiled section? Yes,
and it is now stated in three places that agree: docs/vision.md:257-262 (tiled section, "report
("review", None), never ("absent", None): a rectangle in padding is not evidence that the target
is missing"), docs/vision.md:421-426 (mapping section, same rule with a link back), and
docs/evaluation.md:765-770 ("a mapped-back answer that is only padding (None) as review"). It
matches localization_report's contract exactly: the three outcome kinds are located/absent/
review, ("located", None) is rejected (I checked: ValueError: example 0: box needs top, bottom, left and right edges), and a review is never counted as an absent prediction, so a padding-only
answer lands in present_review/absent_review rather than inventing a miss. It also matches
CONTRIBUTING.md §4 ("Uncertain or inconsistent results go to an explicit review outcome") and the
localization_outcome policy at docs/vision.md:146-156. The builder's choice not to present
image_to_tile as a source of the answer rectangle is correct — it maps gold into a tile, the
opposite direction — and the parenthesis saying so is a useful warning.
Keeping "after tile_to_image" because a test needs it: correct, and the reasoning holds up. A
pre-existing test from #311/#331, test_the_docs_say_a_tiled_recipe_reports_per_image_not_per_tile
(tests/test_vision.py:1217-1236), requires the literal substring after `tile_to_image` in that
section. The phrase is still true for a tile-only recipe, so extending the sentence rather than
replacing the phrase is the right move: it keeps another issue's guard intact and says the whole
truth. Nothing was weakened to make a check pass.
Review of git diff 308cac8 2401cba
Four files, +149/−6, all 100644 → 100644:
:100644 100644 155025a 8707cea M docs/evaluation.md
:100644 100644 6bcd020 a085b49 M docs/vision.md
:100644 100644 a813094 536b9af M src/jev_cookbook/vision.py
:100644 100644 7b1abbe 1bac780 M tests/test_vision.py
vision.py:785-786— the zero-size guard, placed after the mode check and before any arithmetic,
using the module's existing helper and the same message text asgenerate_tiles:488-489. Correct.vision.py:821—Image.frombytes("RGB", prepared.size, prepared.tobytes())with a comment
explaining thatconvert()/resize()copy.infowhile only the pad path starts from a fresh
canvas.preparedis RGB on every path at that point, so the rebuild is type-safe; it costs one
full-size buffer (7.4 MB at 1568²) and I confirmed the identity path is still pixel-identical
(prepared.tobytes() == source.tobytes()), so the docs' "a pixel-identical copy" claim still
holds..info == {}on all five paths I tried (fits / resize / pad / resize+pad / upscale), and
both a PNG and a JPEG saved from the result still passvalidate_image_asset.vision.py:769-772— docstring: metadata-free on any path, orientation not applied. Accurate.- Docs: the four edits described above. The metadata sentences are true as written — I verified a
saved PNG contains exactlyIHDR, IDAT, IEND, and thatimage_assetreally does not inspect PNG
chunks (_image_formatchecks format, stillness, EXIF orientation, dimensions, area and size), so
"the metadata rules aretools/check_recipe_assets.py's" is the right pointer. - Tests:
+111lines. The metadata test is the strongest addition — it loads the repository's actual
checker by path (house pattern), assertscheck_image(...) == []for the prepared image on four
size classes, and then asserts the unprepared source fails with exactly["png-metadata"]and
iCCPin the message. That is an instrument run against a case where it must find something.
The resampling test pins "an averaging filter, not nearest neighbour" without pinning Lanczos's
kernel (I see greys[2, 51, 251]in columns 780–790), which is the right amount of pinning.
The doc-text test pins the new sentences, including the 0.13% figure, which it also re-derives. - Nothing was removed, loosened, skipped or xfailed; no test was weakened to accommodate the change.
PR body's new section against the diff
The appended "After the two reviews" section is accurate on every point I can check, and it corrects
the earlier round honestly: it says the "nine mutations each caught" claim did not cover the then
branch, and that four files are touched, not three. I verified its figures myself: 6312 passed, 55 skipped, 50 deselected, 31 xfailed (my run: identical, 242.81s) and tests/test_vision.py 396
passed (mine: 396, twice). foundation/multimodal-base is indeed gone from the remote
(git ls-remote --heads origin "foundation/multimodal-*" lists only multimodal-api and
multimodal-integration). Two notes, neither about truthfulness:
- The older "Not changed" section still reads "
docs/evaluation.mdline 766 still names
tile_to_image(correct as it stands). No other file was touched", which is now false in
isolation; it is rescued only by the new section's "supersedes the figures and statements above
where they differ". A reader of the body alone meets the contradiction before the resolution. - Editing the body after the final push is what
CONTRIBUTING.md:121tells contributors not to do
("A pull-request description is written once, at open (or with the first push), and never edited
after the final push … Record fix-round notes and integration receipts in pull-request comments
instead"), precisely because theeditedevent fires a secondScoperun. It did: head 2401cba
carries twoScope(pull_request_target) runs,39712008075and39712507631. Both are
success, so the Readiness helper: duplicate successful Scope rows on one head (synchronize + edited) fail closed; decide the rule #138 collapse rule applies and the head is still mergeable — benign here, but
the next such edit may not be, and the note belonged in a comment.
Checks I ran on 2401cba
Fresh Python 3.14.7 venv under /home/agent/ (uv venv --python 3.14,
uv pip install --link-mode symlink -e ".[dev]", Pillow 13.0.0), everything inside a single
flock .claude/heavy.lock nice -n 10 acquisition (no nested lock), pytest -p no:cacheprovider.
| Check | Result |
|---|---|
ruff check . |
All checks passed!, exit 0 |
ruff format --check . |
405 files already formatted, exit 0 |
pytest (full) |
6312 passed, 55 skipped, 50 deselected, 31 xfailed in 242.81s, exit 0 |
pytest tests/test_vision.py ×2 |
396 passed in 4.65s, then 396 passed in 3.66s — same count, not flaky |
python tools/check_hygiene.py |
Hygiene check passed: 615 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 |
exit 0 |
Worked example re-extracted from the new docs/vision.md and run outside its test |
prints the five documented lines, MATCH: True |
| Seven-row size table recomputed independently | all seven sizes and all four quoted content rectangles match, unchanged from round 1 |
12 mutations (my 9 from round 1, a second then variant, the metadata rebuild, the zero-size guard) |
12 of 12 caught. then_wrong_window → test_then_refuses_mismatched_sizes_and_content_that_is_all_padding; then_wrong_window_self → test_a_box_found_two_levels_down_maps_to_the_full_image_exactly; no_metadata_rebuild → the new metadata test; no_zero_size_guard → the new zero-size test; the other eight as in round 1. Baseline on the copied tree: 396 passed. |
8 probes (size table, metadata on five paths incl. PNG chunks and JPEG re-validation, identity pixels, zero sizes, EXIF orientation, 0.13% figure, the pinned then windows, resampling) |
as reported above; the EXIF one is S1 |
| Commit hygiene | signed (G), one commit on 308cac8, Refs #310 in the message, Co-Authored-By: Claude Sonnet 5.5 present; no rebase, no force-push (the first head is still the parent) |
| CI on this exact head | every row green: Lint (ruff), Catalog, Tests (py3.10) 3m14s, Tests (py3.14) 4m41s, Hygiene, Fixtures (validate), Notebooks (discover/execute), Scope; read, not assumed |
No network call, no API key, no live inference, no notebook executed in the tree under review (no
recipe uses these helpers yet).
Acceptance criteria of #310
Unchanged from my first review: all five are met, and the multi-level criterion is now also properly
guarded by tests. The new doc sentences do not disturb any of them; M1 is a disclosure problem in
new prose, not an unmet criterion.
Scope
Four files against origin/main (docs/evaluation.md, docs/vision.md,
src/jev_cookbook/vision.py, tests/test_vision.py), all regular-file modifications, no mode or
type change, no rename or deletion, nothing in catalog/, tools/, .github/ or recipes/.
docs/evaluation.md is new to the diff this round and is a legitimate foundation edit (the
cross-reference I asked for); the body discloses it. No secrets, no absolute home paths.
OPUS-REVIEW: CHANGES-NEEDED 2401cba
Re-review of PR #341 — reviewer 1 of 2 (independent), round 2
VerdictM1 is resolved, and resolved properly. 1. Is M1 resolved?Yes. The fix ( # convert() and resize() copy the source's .info (an ICC profile would become an iCCP chunk
# when saved); only the pad path starts from a fresh canvas. Rebuild from the pixel bytes so
# every path returns an image with no metadata.
prepared = Image.frombytes("RGB", prepared.size, prepared.tobytes())I replayed my own round-1 save/iCCP route rather than trusting the new test. A source carrying
Round 1's failures ( The fix is pixel-neutral, which matters because a changed pixel would move committed asset
The docstring and EXIF orientation: acceptable, not a trapThe new docstring says "Orientation is not applied: rotate the pixels first with I still judge this acceptable, for three independent reasons:
Optional hardening, offered as a suggestion only (S8 below): mirror Mode conversionsAll accepted modes convert cleanly, carry no metadata and pass the contract; everything with an
2. Property tests re-run against the new codeThe geometry functions are byte-identical in this diff, so I expected — and got — results
New this round, because
Edge cases re-run on the new head: NaN, ±inf, negative, >1, zero-width/height, three-edge, 3. Review of
|
| claim | my result |
|---|---|
then()'s derived-window remap is now pinned (three assertions) |
Correct — I re-derived all three by hand (§3) |
prepare_image returns a metadata-free RGB image on every path; .info empty; PNG only IHDR/IDAT/IEND; passes check_recipe_assets.py; docs say image_asset does not check PNG chunks |
Correct on all five counts, verified independently (§1) |
A zero-sized image raises ValueError |
Correct, in all three orientations |
| A test pins that the filter is averaging, not nearest-neighbour, and does not pin Lanczos itself | Correct and precisely stated — NEAREST fails, BILINEAR/BICUBIC/HAMMING pass (§5) |
| four files touched, not three | Correct |
Full pytest 6312 passed, 55 skipped, 50 deselected, 31 xfailed on 3.14 |
Reproduced exactly |
tests/test_vision.py 396 passed on 3.10 |
Reproduced exactly |
foundation/multimodal-base … no longer exists on the remote |
Correct; this is the round-1 staleness, now disclosed |
One precision point, not an overclaim: "the earlier 'nine mutations each caught' did not cover
that branch" is a statement about the author's own mutation set, and is fair. But a reader could
take it to mean the branch was previously unconstrained, and it was not: I ran both of my
second-remap mutants (-> _FULL, and "uses the first transform's windows") against a copy of
308cac8's test file and both were already killed there (4 and 3 failures). What the new
assertions add is a direct assertion of the window's value instead of coverage-by-consequence —
worth having, and more precise, but not first coverage. (Both the temporary test copy and the
mutation were removed; git status clean.)
5. Mutation testing — 33 of 33 killed
Same 27 mutants as round 1 plus 7 new ones aimed at this commit. 34 attempted, 1 skipped (a
non-unique anchor), 33 killed, 0 survived:
- round-1 set, all killed again: never snap / always snap;
_clipcounts touching;_remapswaps
fx/fy;scaledceil / half-down / floor; odd pad pixel left/top; content top-left;min_side
strict;min_sideon the long side;generate_tilesignores a small source;generate_tiles
no check;crop_boxno check;crop_boxchecks after cropping;thenwrong first remap;then
no empty-intersection check;upscaleignoresmax_side;upscaleoverrides a downscale;
_as_boxcollapses;to_originalclips to the wrong window;visible_fractionreturns an area;
nomin_side > max_sidecheck;pad_coloraccepts 256; transparency allowed through;
crop_boundsdrops the expansion. LANCZOS->NEAREST: now KILLED (my round-1 S1, the only survivor then).- new, all killed: the
frombytescopy removed; thefrombytescopy only on the pad path; the
zero-size check removed;then's second remap ->_FULL;then's second remap using the first
transform's windows. - skipped: a width-only variant of the zero-size check (its anchor appears twice in the file);
the full-removal variant above already covers it.
The filter test's scope, measured: with the filter swapped to NEAREST the suite fails; with
BOX it fails (incidentally, on a different test); with BILINEAR, BICUBIC and HAMMING all
396 tests pass. So the test pins the filter family and genuinely does not pin Lanczos's kernel
— which is the right trade-off: it kills the mutant I cared about without making the suite hostage
to a Pillow kernel refinement, and the PR body says precisely that.
The worktree was restored after every mutation run (git status clean, git rev-parse HEAD =
2401cba…).
6. Checks I ran
Fresh venvs under /home/agent/r1b/ (uv venv --python 3.14 / --python 3.10, then
uv pip install --link-mode symlink -r a one-line requirements file holding -e <worktree>[dev]),
every heavy command under flock .claude/heavy.lock nice -n 10, pytest -p no:cacheprovider, no
nested flock, background wrapper + log polling for the long runs, available memory checked first
(4.5 GB).
| command | result |
|---|---|
ruff check . (0.17.1) |
All checks passed!, exit 0 |
ruff format --check . |
405 files already formatted, exit 0 |
pytest (3.14.7, Pillow 13.0.0) |
6312 passed, 55 skipped, 50 deselected, 31 xfailed in 237 s, exit 0 |
pytest tests/test_vision.py (3.10.21, Pillow 12.3.0) |
396 passed in 5.0 s |
python tools/check_hygiene.py |
passed: 615 file(s) scanned, exit 0 |
python tools/check_recipe_assets.py |
passed: 461 file(s) under recipes/ checked, exit 0 |
python tools/render_catalog.py --check |
exit 0 |
python tools/notebook_ci.py fixtures |
51 recipe folders, 0 failed, exit 0 |
| my independent exact-model property tests | 37,362 chains / 45,279 mappings, 0 problems |
| my 34 mutants | 33 killed, 0 survived, 1 skipped |
| filter-family scope (BOX/BILINEAR/BICUBIC/HAMMING/NEAREST) | family pinned, kernel not |
prepare_image digests, Pillow 13.0.0 vs 12.3.0 vs 308cac8 |
all identical |
check_recipe_assets.check_image on prepare_image output, 8 paths, PNG + JPEG |
[] everywhere; unprepared control still png-metadata/iCCP |
400 random sizes: window vs real pixel bbox, limits, .info |
all match |
git merge-tree 23f523e 2401cba |
clean, no conflicts |
No notebooks, recipes or fixtures are touched by the diff, so no notebook execution was needed and
tools/check_recipe_scope.py has nothing to judge.
7. The base moved during this review
origin/main advanced from ebf2413 to 23f523e ("Foundation: bind the confidence gate to
the shared helper and a live review branch (#282, #227, #272) (#336)") while I was running the
suite — another agent fetched it into the shared object store. So:
git merge-base HEAD origin/mainis stillebf2413: Foundation: image preparation, tile inverse, multi-level box mapping and a 280 px minimum check (#310) #341 is now one commit behindmain.git merge-tree --write-tree 23f523e 2401cbaproduced a single tree with no conflicts.main's new commit touchesCONTRIBUTING.md,docs/evaluation.md,docs/recipe-template.md
andtests/test_recipe_guard.py. The only file it shares with this PR isdocs/evaluation.md,
and the two edits are far apart (its insertion is at ~line 276, this PR's at ~line 763);
tests/test_recipe_guard.pyis independent ofvision.py.
I ran every check on the head alone, as my brief instructed. Given the clean merge and the
non-overlapping files I see no risk, but the orchestrator should note that a merge of origin/main
into the branch is now needed before it goes in, and may want the suite re-run on the merged tree.
8. Status of my round-1 findings
| # | Finding | Status |
|---|---|---|
| M1 | prepare_image propagates .info; saved PNG carries iCCP and fails png-metadata on 2 of 3 paths |
Resolved. Clean frombytes copy on every path; .info == {} and check_image(...) == [] on all 8 paths I tested; pixel- and byte-neutral; negative control asserted by the new test; both docs updated. |
| S1 | Nothing pins the resampling filter (my only surviving mutant) | Resolved. NEAREST is now killed; the test pins the family, not the kernel, and says so. |
| S2 | upscale is not type-validated (upscale="yes" is truthy) |
Not changed (still accepted; the docs now explain when to pass it instead). Minor; leaving it is defensible. |
| S3 | EXIF orientation unmentioned | Resolved in documentation — the docstring and docs/vision.md now say orientation is not applied and to use ImageOps.exif_transpose. See §1 for why I accept the documented behaviour, and S8 for optional hardening. |
| S4 | Four added lines over the configured line-length = 100 |
Not changed. vision.py:371 (108), :380 (102), :597 (114) — :140 was pre-existing — plus two 101–102-char test lines and docs/vision.md:307. E501 is not in ruff's select, so nothing flags them. Cosmetic. |
| S5 | The "too small for a float rectangle" raise escapes a None-returning API |
Not changed, and I am content: docs/vision.md already carries the bullet "A mapped box too small to be a float rectangle raises ValueError, never a collapsed box", and I again could not reach it from anything realistic. |
| S6 | ImageTransform's public fields annotated with the private _Edges alias |
Not changed. Cosmetic. |
| S7 | docs/evaluation.md:766 named only tile_to_image |
Resolved — now names the ImageTransform.to_original chain and the None -> review rule. |
| S8 (new, optional) | prepare_image silently drops a non-upright source's orientation tag, so the result passes validate_image_asset where the original file would have been refused |
Suggestion only. Mirroring _image_format (raise when image.getexif().get(274, 1) != 1) would make the one silent case loud for three lines. Not required: the behaviour is unchanged from 308cac8, identical to docs/fixtures.md's own clean-copy snippet, documented in three places, and the input class is banned by CONTRIBUTING.md:92. |
All five acceptance criteria of #310 remain met (round 1, §"Acceptance criteria"), and criteria 3
and 5 are now better pinned: the then chain's derived window is asserted by value, and the docs
state how a mapped-back None is reported.
Nothing in the remaining suggestions would stop me being comfortable with this merging.
OPUS-REVIEW: APPROVE 2401cba
…nce; refuse EXIF orientation in prepare_image (#310) The upscale paragraph now states only the arithmetic (a 10 x 10 crop padded to 280 x 280 is 0.13% content) and that the choice is a design decision not yet measured against Jev. prepare_image drops EXIF, so it now refuses a non-identity orientation the way image_asset does and reports damaged EXIF as a ValueError. Refs #310 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Re-review of PR #341 (round 3) — reviewer 2 of 2 (independent)
Verdict in one paragraphMy blocker is resolved: the clause is gone, and the rewritten paragraph ( 1. Is the blocker resolved?Yes.
Every sentence is now either arithmetic I verified or a disclosure:
Any remaining claim about what Jev can or cannot do? No. Every "Jev" in the two changed files:
The section is now internally consistent with 2. The EXIF change, judged as a recipe builder
try:
orientation = image.getexif().get(274, 1) # the rebuild below drops EXIF and this evidence
except (struct.error, SyntaxError, OSError) as exc:
raise ValueError("the image's EXIF data is damaged; remove it before preparing") from exc
if orientation != 1:
raise ValueError(_ORIENTATION_MESSAGE)Refusing beats silently dropping, and this is the right shape of refusal. In round 2 I measured Does it break a documented workflow? No — I checked each one on the real code:
The message. "rotate EXIF orientation into pixels with ImageOps.exif_transpose and save the Damaged EXIF. The new catch does what it says — I exercised five malformed blobs and got 3.
|
| Check | Result |
|---|---|
ruff check . |
All checks passed!, exit 0 |
ruff format --check . |
405 files already formatted, exit 0 |
pytest (full) |
6457 passed, 65 skipped, 50 deselected, 31 xfailed in 252.27s, exit 0 |
pytest tests/test_vision.py ×2 |
402 passed in 4.88s, then 402 passed in 3.49s — same count, not flaky |
python tools/check_hygiene.py |
Hygiene check passed: 615 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 |
exit 0 |
Worked example + the "Minimum size" snippet, extracted from this head's docs/vision.md and run outside their tests |
example prints the five documented lines, MATCH: True; snippet runs clean |
| Seven-row size table and its four quoted content rectangles, recomputed independently (third round running) | all match, unchanged |
| 9 EXIF/docs probes (remedy, orientation 1 / none / other tags, tile and crop inheritance, asset round trip, five damaged blobs, the shared constant, the 0.13% and upscale figures, zero-size and PNG chunks) | all as reported above; PROBES_OK |
| 15 mutations | 15 of 15 caught. New this round: no_orientation_refusal → test_prepare_image_refuses_a_non_identity_exif_orientation_like_image_asset; orientation_default_zero (.get(274, 1) → .get(274, 0)) → test_prepare_image_pads_one_axis_and_keeps_every_pixel; removing the damaged-EXIF try/except → test_prepare_image_reports_damaged_exif_as_a_value_error with the raw struct.error the guard converts. The twelve from rounds 1–2 (both then windows, snapping, half-up rounding, pad parity, min_side on tiles and crops, strict minimum, touch-as-overlap, unbounded upscale, the metadata rebuild, the zero-size guard) still all fail. Baseline on the copied tree: 402 passed. |
| Commit hygiene | both new commits signed (G); b46d935 ends Refs #310 + Co-Authored-By: Claude Sonnet 5.5; merge commit carries the co-author trailer; parentage shows no rebase, force-push or amend |
| CI on this exact head | All green. Lint (ruff), Catalog (README is current), Hygiene, Fixtures (validate), Notebooks (discover) and Scope (recipe pull requests) passed on my first read, when Tests (py3.10), Tests (py3.14) and Notebooks (execute) were still running (the head was minutes old); I waited and re-read them: Tests (py3.10) pass 5m7s, Tests (py3.14) pass 5m18s, Notebooks (execute) pass. Read from the PR, not assumed. |
No network call, no API key, no live inference, nothing executed in the tree under review beyond the
read-only doc snippets, no notebook affected (no recipe uses these helpers yet).
Scope
Against origin/main (717a3f0), four files, all regular-file modifications, no mode or type
change, no rename or deletion, nothing in catalog/, tools/, .github/ or recipes/:
:100644 100644 602a56f b90268c M docs/evaluation.md
:100644 100644 57990b5 cdf22c1 M docs/vision.md
:100644 100644 959bfe9 3310e29 M src/jev_cookbook/vision.py
:100644 100644 fe139f1 7da9ae0 M tests/test_vision.py
Foundation branch, so the recipe allowlist does not apply; Scope (recipe pull requests) passes.
No secrets, no absolute home paths, no credential reference. Nothing was skipped, xfailed, loosened
or deleted to make a check pass — and the one existing test whose text constrains this work
(test_the_docs_say_a_tiled_recipe_reports_per_image_not_per_tile, which requires the literal
after `tile_to_image` ) was satisfied by extending the sentence rather than by editing the test.
Earlier findings, final status
| Finding | Status |
|---|---|
R1 M1 then() derived-window remap unpinned |
Resolved in 2401cba, still pinned here (my mutant fails; a second variant fails too) |
R1 S1 reporting guidance named only tile_to_image |
Resolved in both docs/vision.md:254-262 and docs/evaluation.md:808-814, and the merge preserved the evaluation hunk byte for byte |
R1 S2 no outcome named for a None from to_original |
Resolved; ("review", None) in three agreeing places, consistent with localization_report's three outcome kinds |
R1 S3 no guidance on choosing upscale |
Resolved, and now stated without any claim about Jev |
R1 suggestion 1 ZeroDivisionError on a zero-sized image |
Resolved; removing the guard turns the file red |
| R2 M1 "too little for Jev to use" | Resolved (section 1 above) |
R2 S1 silently dropping an EXIF orientation image_asset refuses |
Resolved, better than I asked: refused, with a shared message and tests from both call sites |
R2 S2 _asset_checker() re-execs the tool per case |
Not taken; trivial, matches house style, I do not press it |
R1 suggestions 2–8 (message wording, the "tile bounds" label, from_crop naming, crop+transform ergonomics, the ULP sliver, long lines, the min_side rationale) |
Not taken, all explicitly non-blocking; the only new long line is a 104-character anchor link at docs/vision.md:258. The 'F'-mode message nit from R1 suggestion 2 is now joined by the two EXIF-message nits in section 2 — one future tidy-up, nothing more |
Acceptance criteria of #310
All five met, unchanged from round 1, and now more tightly guarded: the multi-level criterion by the
three then() assertions, the min_side criterion by the before-any-crop tests, the prepare_image
criterion additionally by the metadata, zero-size and EXIF tests, and the worked-example criterion by
a byte-for-byte output comparison that I also ran outside its test.
OPUS-REVIEW: APPROVE 3b01fc1
|
Record update for the final head 3b01fc1 (a comment, not a body edit, since the head is final). Supersedes the PR body where they differ:
|
Re-review of PR #341 — reviewer 1 of 2 (independent), round 3
VerdictThe merge is a pure auto-merge (its tree is bit-identical to the one 1. The merge commit (
|
| orientation in a JPEG | prepare_image |
validate_image_asset |
|---|---|---|
| 1 | ACCEPT | ACCEPT |
| 2, 3, 4, 5, 6, 7, 8 | ValueError: rotate EXIF orientation into pixels… |
the same ValueError |
| 0, 9, 255, 65535 (malformed) | the same ValueError |
the same ValueError |
Identical on all three Pillow versions. Refusing an out-of-range value is the same thing
_image_format has always done (!= 1), so the two cannot diverge — which is the point of the
shared constant. The agreement is behavioural, not just a shared string.
3.2 IFD layouts and byte order
| case | getexif() sees |
prepare_image |
image_asset |
|---|---|---|---|
little-endian II TIFF header, hand-built, orientation 1 / 3 / 6 / 0 / 9 |
1 / 3 / 6 / 0 / 9 | ACCEPT / refuse x4 | — |
big-endian MM TIFF header, hand-built, same values |
1 / 3 / 6 / 0 / 9 | ACCEPT / refuse x4 | — |
orientation only in the Exif sub-IFD (0x8769) |
absent in IFD0 | ACCEPT | ACCEPT |
| orientation as a RATIONAL, and an entry with a 2^31 offset | a non-1 value | refuse | refuse |
orientation with count=3 (Pillow warns) |
1 | ACCEPT | ACCEPT |
The sub-IFD case is a blind spot, but it is image_asset's blind spot too — the two still agree,
and orientation there is non-standard. Nothing diverges.
3.3 Damaged, truncated and garbage EXIF
Seventeen malformed blobs assigned to info["exif"] (header only; Exif\0\0II*\0; a partial
offset; an offset past the end; a truncated MM*; bad magic; an entry count with no entries; a
65,535 entry count; random bytes; all zeros; empty; a raw TIFF with no Exif prefix; a NUL run
after a valid head; a 2^31 entry offset; a RATIONAL orientation; a count-3 orientation;
bytearray; memoryview):
- every byte-like blob either parses (Pillow only warns
Corrupt EXIF data) or raises
struct.errororSyntaxError, and the newexceptturns both into the documented
ValueError: the image's EXIF data is damaged; remove it before preparing.
No leak, on any of the three Pillow versions. The caught tuple is exactly right. - the only escapes are non-byte-like values:
info["exif"]set to astr, anintor a
listleaksTypeError/AttributeError, and on Pillow <= 12.3 amemoryviewleaks
AttributeErrortoo. That requires caller code to assign a wrong type — Pillow always stores
bytesthere when opening a JPEG, PNG or WebP — so it is a programming error, and the
exception type is not misleading. Suggestion S9 below, one word; not a blocker.
I also checked the neighbouring exposure, because _image_format's own try does not list
struct.error: I pushed all fifteen saveable blobs through real JPEG files and
validate_image_asset. No non-ValueError escaped — Pillow rewrites/pads the APP1 segment on
save and only warns on reopen. So there is no pre-existing hole to report there.
3.4 EXIF from other container formats, and XMP
| source | getexif() orientation |
prepare_image |
|---|---|---|
PNG with an eXIf chunk (orientation 6) |
6 | refuse (and that PNG independently fails the contract with png-metadata) |
| WebP with EXIF (orientation 6) | 6 | refuse |
XMP-only tiff:Orientation="6" (Pillow's getexif falls back to XMP) |
6 | refuse |
The XMP case is worth calling out as a good consequence of using the same public getexif()
as _image_format: both refuse it, so they still agree. getexif() and the deprecated private
_getexif() returned the same orientation (6) for a JPEG, so the choice of API changes nothing
except that it matches _image_format exactly.
3.5 Does it refuse anything the repository's rules would accept? No.
| input | verdict |
|---|---|
Image.new("RGB", …) (how every repo asset is drawn) |
ACCEPT |
| a plain PNG saved and reopened | ACCEPT |
| EXIF present but no orientation tag; orientation 1 | ACCEPT |
ImageOps.exif_transpose(...) output, orientations 2/3/6/8 |
ACCEPT |
the repo's own flow: draw a 1200x900 page -> save -> reopen -> generate_tiles(min_side=280) -> crop_box -> prepare_image |
ACCEPT, info == {} |
And nothing that could legally be a committed asset can carry an orientation tag at all:
image_asset already refuses a non-identity orientation, and tools/check_recipe_assets.py
forbids eXIf in a PNG and every non-JFIF APPn in a JPEG. So prepare_image is no stricter
than the asset rules for anything that could be an asset; for a source image it refuses sideways
pixels, which is the intent. The full suite passing (6457 tests) is the other side of this: nothing
in the repository regressed.
3.6 The documented remedy works end to end
For each of orientations 2, 3, 6 and 8: ImageOps.exif_transpose rotates the pixels
(400x300 -> 300x400 for 6 and 8) and removes tag 274, prepare_image then accepts, and the
saved PNG gives image_asset = ACCEPT and check_recipe_assets.check_image(...) == []. A crop of
an exif_transpose'd source is accepted too. This was the check that mattered most — if the
prescribed remedy had still been refused, the change would have been a blocker.
Conversely, a crop() or a generate_tiles tile of a rotated source inherits the parent's
info["exif"] and is refused. That is the right answer (its pixels are sideways), and it means
the refusal propagates correctly through the tiling path rather than being bypassed by cropping.
3.7 Input mutation, and two notes on Pillow's _exif cache
prepare_image leaves the input's pixels, size and info byte-identical; only Pillow's private
_exif lazy cache is populated, which is not observable through the public API. The round-1 test
test_prepare_image_does_not_modify_the_input_and_returns_new_pixels still holds.
Two measured quirks of that cache, reported as observations with no change recommended:
- Not idempotent on damaged EXIF. Three calls in a row on the same image object give
ValueError("the image's EXIF data is damaged…"), then ACCEPT, then ACCEPT, because a failed
Exif.loadleaves_exifmarked loaded. For a well-formed non-identity orientation all three
calls refuse identically, so the case that matters is stable. Reaching this needs damaged EXIF,
which the repository's code-generated assets cannot have, and the first call does refuse. - A stale cache can hide a real orientation, but only in the contrived order "call
getexif()
on an image that has no EXIF yet, then attachinfo['exif']". A file opened from disk and
read first still refuses (I checked).
I deliberately do not suggest replacing image.getexif() with a fresh Image.Exif().load(...)
to fix these: that would drop the XMP fallback and make prepare_image disagree with
image_asset on the case in §3.4 — worse than the quirk. Agreement is the more valuable property
and the current code buys it.
4. Property tests re-run
Same seed, same independent exact-rational model built from scratch; the geometry functions are
untouched in this diff and the results are bit-identical to rounds 1 and 2:
| property | cases | result |
|---|---|---|
random chains of 1–3 crop / resize+pad steps built with then |
37,362 | all built; only the documented empty-intersection raise |
to_derived vs my exact model (float equality) |
20,338 | 0 mismatches |
to_original vs my exact model (float equality) |
24,941 | 0 mismatches |
to_derived returning None where my model says empty |
14,126 | 0 disagreements |
then associativity (left fold == right fold) |
37,362 | 0 differences |
visible_fraction vs exact area arithmetic |
single-step subset | 0 differences |
| total problems | — | 0 |
5. Mutation testing — 24 of 24 killed
The key mutants from rounds 1–2 plus seven new ones aimed at this commit and one aimed at the
docs guard. All 24 killed, 0 survived.
- geometry core, killed again: never snap / always snap;
_clipcounts touching;_remapswaps
fx/fy;thenwrong first remap;thenwrong second remap;thenno empty-intersection check;
scaledhalf-down; content top-left not centred;min_sidestrict;crop_boxno check;
to_originalclips to the wrong window;visible_fractionreturns an area;NEARESTinstead
of Lanczos; thefrombytescopy removed; the zero-size check removed. - new, all killed: the whole orientation check removed; the refusal inverted (
== 1); the
default for a missing tag changed to0(so a no-EXIF image would be refused); the wrong tag
number (275); the damaged-EXIFexceptremoved sostruct.errorleaks;prepare_imagegiven
its own message instead ofimage_asset's; the check disabled withand False. - docs guard, killed: putting the removed claim back ("too little for Jev to use") fails
test_the_docs_say_how_to_report_a_mapped_back_answer_and_when_to_upscale. So the negative
assertion"too little for Jev" not in flatis a real guard, not decoration.
The worktree and docs/vision.md were restored after every mutation (git status clean,
git rev-parse HEAD = 3b01fc1…).
I also re-read the three new tests as tests. They are real checks, not restatements: the
orientation test asserts image.getexif()[274] == orientation first (so the fixture really
carries the tag), then asserts both prepare_image and image_asset refuse the same file
with the same message — which is exactly the property the shared constant is meant to guarantee,
checked through two independent entry points. The acceptance test covers both orientation 1 and no
EXIF. The damaged-EXIF test uses a header-only blob, which I confirmed is one of the inputs that
genuinely makes getexif() raise struct.error on all three Pillow versions, so it exercises the
new except rather than passing vacuously. The metadata test's EXIF blob is now a real
Image.Exif() with an ImageDescription — a better fixture than the old truncated header, and
one that does not accidentally depend on the damaged-EXIF path.
6. The docs paragraph (criterion 4)
Choose
upscaleonce per recipe. The default pads, which keeps every original pixel and clears
the 280 pixel minimum. A small crop is then almost all padding, and padding adds no detail: a
10 × 10 crop comes back 280 × 280 with the content covering 0.13% of the image, the target no
larger than it was in the source.upscale=Trueinstead scales the crop up so the target fills
more of the image sent. Which of the two works better for a recipe has not been measured against
Jev; treat it as a design decision to settle on validation examples, then keep it, like
pad_color, the same across the recipe's images.
Every clause is arithmetic or mechanical — "keeps every original pixel" (upscale=False does not
resample a small crop), "clears the 280 pixel minimum", "padding adds no detail", "0.13% of the
image" (100/78,400 = 0.1276%, pinned by a test), "the target no larger than it was in the source"
(scale factor 1), "scales the crop up so the target fills more of the image sent" — and the only
statement about which is better is explicitly "has not been measured against Jev; treat it as a
design decision to settle on validation examples". The removed claim is gone and its absence is
asserted. I grepped every added documentation line in the whole PR for "Jev": the remaining
mentions are the pad_color sentence (already hedged with "has not been measured against Jev"),
"a recipe that sends tiles or crops to Jev", "A box that Jev returns", "where a recipe would call
Jev" and two import lines — no unmeasured behavioural claim about Jev is left anywhere in the
PR's added prose. Criterion 4 met.
7. Checks I ran
Fresh venvs under /home/agent/r1c/ (uv venv --python 3.14 / --python 3.10, then
uv pip install --link-mode symlink -r a one-line requirements file holding -e <worktree>[dev]),
every heavy command under flock .claude/heavy.lock nice -n 10, pytest -p no:cacheprovider, no
nested flock, background wrapper + log polling for the long runs, memory checked first (3.9 GB).
| command | result |
|---|---|
ruff check . (0.17.1) |
All checks passed!, exit 0 |
ruff format --check . |
405 files already formatted, exit 0 |
pytest (3.14.7, Pillow 13.0.0) |
6457 passed, 65 skipped, 50 deselected, 31 xfailed in 252 s, exit 0 |
pytest tests/test_vision.py (3.10.21, Pillow 12.3.0) |
402 passed in 5.7 s |
python tools/check_hygiene.py |
passed: 615 file(s) scanned, exit 0 |
python tools/check_recipe_assets.py |
passed: 461 file(s) under recipes/ checked, exit 0 |
python tools/render_catalog.py --check |
exit 0 |
python tools/notebook_ci.py fixtures |
51 recipe folders, 0 failed, exit 0 |
| my independent exact-model property tests | 37,362 chains / 45,279 mappings, 0 problems |
| my 24 mutants (7 new, EXIF-focused) | 24 killed, 0 survived |
EXIF battery: orientations, II/MM, sub-IFD, 17 damaged blobs, PNG/WebP/XMP, remedy, crops |
no leak for byte-like input; prepare_image and image_asset agree everywhere |
| the same battery on Pillow 11.3.0, 12.3.0, 13.0.0 | identical behaviour for byte-like input |
git merge-tree b46d935 717a3f0 vs the merge commit's tree |
identical OID cff1e14… |
Counts move as they should: tests/test_vision.py is 396 + 6 = 402, and the full suite is
6312 (round 2) + 6 (this commit) + 139 from main's #336 and #344 = 6457, with the 10 extra
skips coming from main. No notebooks, recipes or fixtures are touched, so no notebook execution
was needed and tools/check_recipe_scope.py has nothing to judge.
8. Remaining suggestions — all minor, none blocking
| # | Suggestion |
|---|---|
| S9 (new) | except (struct.error, SyntaxError, OSError) at src/jev_cookbook/vision.py:789 does not cover a non-byte-like info["exif"], which leaks TypeError (a str/int/list on all versions) or AttributeError (a memoryview on Pillow <= 12.3). Adding TypeError, AttributeError to the tuple would keep the module's "bad input raises ValueError" convention. Only reachable from caller code that assigns a wrong type; Pillow always stores bytes. |
| S2 | upscale is still not type-validated (upscale="yes" is accepted as true) while min_side, max_side and pad_color are all strict. Addressed as documentation instead; defensible. |
| S4 | src/jev_cookbook/vision.py:371 (108), :380 (102), :597 (114) exceed the configured line-length = 100 (:140 is pre-existing), plus two 101–102-char test lines. E501 is not in ruff's select, so nothing flags them. Cosmetic. |
| S6 | ImageTransform's public fields are annotated with the private _Edges alias. Cosmetic. |
| note | The two _exif cache quirks in §3.7, for which I recommend no change. |
9. Status of every finding across the three rounds
| # | Finding | Status |
|---|---|---|
| M1 (round 1) | prepare_image propagated .info; a saved PNG carried iCCP and failed png-metadata on 2 of 3 paths |
Resolved in 2401cba, re-verified then; untouched here. |
| S1 (round 1) | Nothing pinned the resampling filter | Resolved in 2401cba; NEAREST still killed here. |
| S3 / S8 (rounds 1–2) | EXIF orientation: first undocumented, then documented but silently laundered away | Resolved here. prepare_image now refuses a non-identity orientation with image_asset's own message, via a shared constant, and reports damaged EXIF as a ValueError. Verified on three Pillow versions, and the documented remedy works end to end. |
| S7 (round 1) | docs/evaluation.md named only tile_to_image |
Resolved in 2401cba; survived the merge with main's edit to the same file. |
| reviewer 2's blocker | The upscale paragraph claimed a padded crop is "too little for Jev to use" |
Resolved here, and the absence of the claim is asserted by a test that my mutant proved fires. |
| S2, S4, S6, S9 | minor, listed in §8 | Open; none blocking. |
All five acceptance criteria of #310 remain met (round 1, with criteria 3 and 5 better pinned in
round 2). I would be comfortable with this merging as it stands; the branch is up to date with
main and every check is green on the merged tree.
OPUS-REVIEW: APPROVE 3b01fc1
Closes #310
What this adds
Everything is in
src/jev_cookbook/vision.py,tests/test_vision.pyanddocs/vision.md.prepare_image(image, *, min_side=280, max_side=1568, pad_color=(128, 128, 128), upscale=False)returns(prepared, transform). It scales a too-large image down (aspect kept, exact ratio rounded half up, Lanczos), optionally scales a small one up (upscale=True, never pastmax_side), then pads a side still undermin_sidewithpad_color, content centered (odd pixel right/bottom). The result is RGB with both sides in[min_side, max_side]. Transparency or an unusual pixel mode raises.ImageTransform(frozen dataclass):crop(bounds, width, height),to_original(box),to_derived(box),visible_fraction(box)andthen(other)for chaining any number of levels. Arithmetic usesfractions.Fractionand rounds to float once, so a multi-level mapping is the true position rounded once. A normalized edge that is the float nearestk / widthis read as pixelk, so a box that touches a tile edge givesNone, not a 1e-17 sliver. A mapped box too small for a float rectangle raises instead of collapsing.image_to_tile(box, tile_bounds, width, height): the inverse oftile_to_image, returning the clipped tile-local box orNone.crop_bounds(box, width, height, *, expand=0.0): the pixel boundscrop_boxcuts, so a crop can be given a transform.crop_boxnow uses it.generate_tiles(..., min_side=None)andcrop_box(..., min_side=None): withmin_sideset, a tile or crop with a side below it raisesValueErrorbefore anything is cropped. The defaultNonechecks nothing.docs/vision.md: new sections "Minimum size for tiles and crops", "Preparing an image for the API", "Mapping boxes between an image and what you sent" and a runnable "Worked example: tile, choose, crop, prepare, Box, map back". The example is executed and its printed output compared by a test. The old sentence "Padding or further cropping requires its own transform" is replaced.tile_to_imageis unchanged apart from its docstring.Decisions for the reviewers
min_sidedefaults toNone(opt-in), not 280, because existing tests and the geometry functions legitimately run on tiny images; changing the default would force edits to existing tests. The docs tell a recipe that sends tiles or crops to passmin_side=IMAGE_LIMITS["min_side"]or useprepare_image. Should the default be 280 in a follow-up?crop_boxandgenerate_tilesrefuse rather than pad. Padding changes the image and needs a transform returned, which is whatprepare_imagedoes.upscale=Falseby default (pad, do not invent pixels). The issue's quoted docs allow either.pad_colordefault: a neutral choice that has not been measured against Jev; the docs say so.prepare_imagecenters the content and returns RGB; an image with an alpha channel, a palette with transparency, or a mode such asF/I;16raises rather than silently flattening.a.then(b).then(c)works for any mix.normalized_to_pixelsrule but applies to the box edge as the floatk / width; it changes only boxes within one float step of a pixel edge.foundation/vision-utilities,foundation/multimodal-api,foundation/multimodal-base,foundation/multimodal-integration) were read; none contain a resize/pad/inverse helper, so nothing was reused.How it was verified
Python 3.14 venv, under the heavy-job lock:
ruff check .exit 0;ruff format --check .exit 0 (405 files formatted, including the code blocks indocs/vision.md).pytest: 6305 passed, 55 skipped, 50 deselected, 31 xfailed (the skips, deselects and xfails are all pre-existing), exit 0.python tools/check_hygiene.py,python tools/check_recipe_assets.py,python tools/render_catalog.py --check,python tools/notebook_ci.py fixtures: each exit 0.pytest tests/test_vision.py389 passed.tests/test_vision.py(no pixel snapping, ceil padding, strictmin_side, half-down rounding, wrong window inthen, touching counted as overlap, missing tile check, upscale ignoringmax_side, float error in the remap).Not changed
docs/evaluation.mdline 766 still namestile_to_image(correct as it stands). No other file was touched.After the two reviews (head 2401cba; supersedes the figures and statements above where they differ)
then()'s derived-window remap is now pinned (three assertions intest_then_refuses_mismatched_sizes_and_content_that_is_all_padding); the earlier "nine mutations each caught" did not cover that branch.prepare_imagereturns a metadata-free RGB image on every path (a cleanImage.frombytescopy): its.infois empty and a saved PNG has onlyIHDR,IDAT,IEND, so it passestools/check_recipe_assets.py; the docs sayimage_assetdoes not check PNG chunks. A zero-sized image raisesValueError. A test pins that the resample filter is averaging, not nearest-neighbour (it does not pin Lanczos itself).docs/vision.mdanddocs/evaluation.md(four files touched, not three) say aNonefromImageTransform.to_originalis reported as("review", None), never("absent", None), and when to passupscale=True(a 10x10 crop padded to 280 is 0.13% content).image_to_tilemaps a gold box into a tile; it is not a source of the per-image answer rectangle.pytest: 6312 passed, 55 skipped, 50 deselected, 31 xfailed on 3.14;tests/test_vision.py396 passed on 3.10.foundation/multimodal-base, named above as an earlier branch, no longer exists on the remote.