Skip to content

Round-2 audit fixes: strict v2 position_id (A-m5), physical-size read errors (A-m6), exact unit conversion (B-m7) - #142

Merged
guijacquemet merged 6 commits into
mainfrom
claude/fix-audit2
Nov 3, 2026
Merged

guijacquemet merged 6 commits into
mainfrom
claude/fix-audit2

Conversation

@guijacquemet

@guijacquemet guijacquemet commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Three fixes from the cross-brick round-2 audit (3 November 2026). An independent review follows, so please do not merge yet.

Summary (by module)

A-m5: position_id is required in v2 manifests

  • New module manifest_fields.py (pure). It holds recorded_position_id(value, relative_path, strict=) and the roles check, which moved here from manifest.py.
  • In version 2, the reader (_file_entry) and the writer (_file_dict) refuse three cases, and each message names the entry and the identifier its name gives:
    • a missing or null position_id;
    • a value that is not text;
    • any value other than position_id(relative_path) (D85: the image name without its extension, never parsed).
  • Version 1 manifests are read as before.
  • run_inputs._fov no longer turns a missing id into "". It raises RunInputsError instead.
  • manifest.py goes from 598 to 592 lines.
  • rc4 compatibility: tests/data/rc4_run_manifest.json was written by the v0.4.0rc4 code (git archive v0.4.0rc4, run through tests/data/make_rc4_manifest.py). The test reads it. Its names cover .tif, .TIFF, .ome.tif, a name with dots, a space and .ome.zarr.
  • position_id was always filled by the writers: every rc tag (rc1 to rc4) sets it through file_entry_with, and image_names.py has not changed since rc1.

A-m6: TIFF calibration read errors (verified first)

  • What was wrong before the fix:
    • a missing, truncated or non-TIFF file gave all-None sizes with no reason;
    • a programming error inside the reader was swallowed in the same way;
    • when tifffile cannot read an XResolution tag (value offset beyond the end of the file, or an unknown type), it drops the tag and only logs. X then read as uncalibrated with no reason, while Y stayed 0.5.
  • New module tiff_size_values.py (io) with two documented exception sets:
    • TIFF_OPEN_ERRORS = (OSError, ValueError, TypeError, struct.error) guards only the tifffile.TiffFile(path) call. Fuzzing tifffile 2026.10.15 showed that it raises TypeError and struct.error on some damaged IFDs, as well as TiffFileError.
    • METADATA_READ_ERRORS = (OSError, struct.error, TiffFileError) covers reading the metadata of an open file.
  • Both give all-None sizes with the reason in PhysicalSizes.invalid, which produces the invalid_physical_calibration warning.
  • A file with no metadata is still simply uncalibrated.
  • Any other exception is raised. A non-path argument is a TypeError, because tifffile raises ValueError for one.
  • A lone ImageJ resolution tag now names the missing one.
  • The value helpers moved to the new module, with full annotations (5 fewer noqa: ANN). physical_sizes.py goes from 600 to 566 lines, and physical_sizes.read_physical_sizes is still importable (MorphoRelate imports it).

B-m7: exact unit conversion

  • length_units._MICROMETERS is now a table of (multiplier, divisor) integers.
  • Units smaller than µm divide by the integer inverse factor; larger units multiply by the integer factor. 9 nm now gives 0.009; before it gave 0.009000000000000001.
  • This repository's own non_square_pixels check uses exact equality too, so X in nm with Y in µm for the same size no longer warns. There is a test for it.

Decisions

  • A-m6 changes inspect_tiff only for tag-read errors. These now become the invalid_physical_calibration warning instead of an unreadable file. A file that cannot be opened is still unreadable_tiff in the folder check.
  • B-m7 follows the requested divide/multiply rule, which gives these guarantees:
    • to_micrometers(x, "nm") == x / 1000 for every value tested;
    • a value that is exact in binary gives the double nearest its exact length in µm, the same in every unit where it is exact, and equal to that length written in µm ("0.009").

Notes for review

  • Remaining 1-ULP gap (B-m7). A value that is not exact in binary (for example 0.1 nm or 108.33 nm) can still differ by 1 ULP from the same length written in µm. In a seeded sample of decimals with up to 6 places, this happened for about 21% of nm values and 11% of mm values.
    • Removing the gap would take a decimal conversion through repr. That would break the requested == x/1000 property, so I did not do it.
    • The 0.1% rule (same_pixel_size) is not affected. Exact-equality tests in other bricks are affected only for such values.
  • Existing tests changed:
    • test_run.py: a test deleted position_id from a v2 manifest and expected every row to be reused. That manifest is now refused (every image is read again). The table case stays in test_run.py; the manifest case moved to test_manifest_position_id.py.
    • The test helper in test_manifest.py now fills position_id.
    • The numpy-scalar test in test_audit_run_inputs.py now expects / 1000.
  • Golden outputs: no fixture or golden output depends on the old conversion. Manifests record each size in its own unit, and the v2 CSV fixtures contain no µm values. The full suite passes unchanged apart from the tests listed above.
  • tools/code_rules.py is unchanged (SHA prefix cc04a420). The baseline only shrank: noqa ANN from 51 to 46, tests/test_run.py from 862 to 860 lines.
  • No new code uses noqa. ruff check --ignore-noqa passes on every new file.

Tests

  • ruff check .: all checks passed. ruff format --check .: 221 files already formatted.
  • python tools/check.py --rules: passed.
  • pytest with .[dev,notebook,zarr] (Python 3.13, Linux): 2138 passed, 4 skipped. The skips are expected: 3 case-sensitive file system checks and 1 file-permission check, because the tests run as root.
  • New tests:
    • test_exact_unit_conversion.py (11)
    • test_manifest_position_id.py (15)
    • test_tiff_size_read_errors.py (19)
  • Without the fixes, 10 of the 14 original A-m5 tests, 17 of the 19 A-m6 tests and 6 of the 10 original B-m7 tests failed.
  • CI on e4b73b6: all 7 jobs passed (lint, and Ubuntu, Windows and macOS on Python 3.11 and 3.13).

🤖 Generated with Claude Code

https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc

…it B-m7)

to_micrometers multiplied by a binary approximation of the unit's factor, so
9 nm became 0.009000000000000001 µm while 0.009 µm stayed 0.009, and an exact
square-pixel test called one size given in nm and in µm non-square. A unit
below the micrometre now divides by its integer inverse factor and a larger
one multiplies by its integer factor: one correctly rounded step, so a value
exact in binary gives the double nearest its exact length in every unit.

Tests check the operation for every unit over 40,000 values, that one length
gives one double in every unit where it is exact, and that X in nm with Y in
µm of the same size is not non_square_pixels. The CHANGELOG notes that sizes
in µm from pm, Å or nm may change by 1 ULP.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
…dit A-m5)

The v2 reader accepted a file entry without position_id or with null, and
RunInputs then keyed that position by ""; an edited position_id was read as
given. Version 2 now requires it and refuses any value other than
position_id(relative_path) (the name without its extension, D85), naming the
entry and the identifier its name gives; the writer refuses the same, so a
written manifest always opens. RunInputs no longer substitutes "".

The checks live in the new pure module manifest_fields (manifest.py was at
598 lines), with the roles check moved there. A manifest written by
v0.4.0rc4 (tests/data/rc4_run_manifest.json, from make_rc4_manifest.py run
with the rc4 code) is read in the tests; v1 manifests are read as before.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
test_run's position test deleted position_id from a v2 manifest and expected
every row reused; that manifest is now refused, so the run reads every image
again. The table case stays in test_run; the manifest case moves to
test_manifest_position_id.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
…-m6)

read_physical_sizes caught every exception and logged it at debug level, so
a file it could not read, or a programming error, read as uncalibrated.
Verified: a missing, truncated or non-TIFF file gave all-None sizes with no
reason, and a resolution tag tifffile drops (value beyond EOF, unknown type;
it only logs) left X silently uncalibrated next to a calibrated Y.

Now opening errors (TIFF_OPEN_ERRORS: OSError, ValueError incl.
TiffFileError, and the TypeError and struct.error tifffile raises on damaged
IFDs, found by fuzzing) and metadata read errors (METADATA_READ_ERRORS:
OSError, struct.error, TiffFileError) give no sizes with the reason in
PhysicalSizes.invalid; a lone resolution tag names the lost one; any other
exception is raised; a non-path is a TypeError. No metadata stays unknown.

The value helpers move to the new module tiff_size_values (physical_sizes.py
was at 600 lines), with full annotations: 5 fewer noqa ANN in the baseline.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
@guijacquemet
guijacquemet marked this pull request as ready for review November 3, 2026 12:54
… is one double (B-m7)

Dividing by the integer factor still left about a fifth of nm decimals 1 ULP from
the same length written in µm (108.37 / 1000 is 0.10837000000000001). to_micrometers
now moves the point of repr(value) by the unit's power of ten and rounds once with
float: no binary factor and no decimal context. For a decimal of at most 15
significant digits this is the decimal written, so the same length in pm, Å, nm,
µm, mm, cm or m gives the double float gives for it in µm. Overflow and underflow
are still refused.

Tests: exhaustive 4-digit decimals in every unit, seeded 1-15 digit decimals, the
16/17-digit limit, seeded doubles over the whole range against an exact Decimal
oracle (deterministic, monotone, refused beyond the range), decimal-context
independence.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
…docstring

A position_id differing only in case or whitespace from its name was not tested
(a casefold or strip comparison survived); the RunInputs guard, the documented
open-error set and the unit gate of a lone resolution tag had no test. The moved
_imagej_text docstring had its µ escape turned into a literal µ.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc

Copy link
Copy Markdown
Contributor Author

Independent review (head e4b73b6, then 47334dd and 28bed66 pushed by the reviewer).

A-m5: approved. Every writer since v2 (d9b109a, 1 Nov) records position_id(relative_path). image_names.position_id and its suffixes have not changed since v2 was introduced, so no manifest from rc1–rc4 or main is refused. I regenerated tests/data/rc4_run_manifest.json from git archive v0.4.0rc4 with the committed script, and it is identical apart from written_at. The reader, the writer and sources.py all use the same D85 function, and the value is compared, never parsed.

A-m6: approved. I fuzzed about 6,800 truncated or byte-flipped ImageJ and OME TIFFs: no exception escaped read_physical_sizes. In inspect_tiff (preflight, check and run), a defect still makes the file unreadable_tiff, so it is never silently uncalibrated. Remaining unexplained unknowns: (a) an ImageJ file with a unit whose two resolution tags were both lost; this is indistinguishable from "a unit without a size", which the contract says is not reported. (b) A TIFF with no pages. Both are minor and left as they are.

B-m7: changed to the shortest-decimal conversion. _shifted moves the decimal point of repr(value) by the unit's power of ten, as text, and float rounds once. It does not use the decimal context. The proposed Decimal(repr(x)) * Decimal(10)**k is context-dependent: with prec=3, 108.33 nm gives 0.108. Verified:

  • deterministic and monotone for every finite positive double;
  • the correctly rounded value of the shortest decimal;
  • within the float range or refused, as before.

"The same length in any unit gives the same double" holds for decimals of up to 15 significant digits. The claimed 17 is wrong: at 16 or 17 digits, two decimals can parse to the same double. Example: 0.95408556734169085 nm. Equality then holds exactly when the decimal written in each unit is the shortest repr of its double, and the tests check that property. x/1000 failed for 24.9% of the 2-decimal nm values from 0.01 to 999.99. The new conversion fails for 0%.

Tests. I strengthened tests where mutants survived: case-only or whitespace-only position_id, the RunInputs guard, the open-error set, and the unit gate of a lone resolution tag. All 22 mutants are now killed. I also restored the µ escape in the _imagej_text docstring.

Gate:

  • ruff check and format: clean;
  • ruff --ignore-noqa: 65 findings before, 60 after;
  • tools/check.py --rules: OK;
  • code_rules.py SHA: cc04a420;
  • pytest: 2183 passed, 4 skipped (3 case-sensitive file system, 1 root permissions), 0 failed;
  • CI on 28bed66: 7/7.

Generated by Claude Code

@guijacquemet
guijacquemet merged commit 59fd799 into main Nov 3, 2026
7 checks passed
@guijacquemet
guijacquemet deleted the claude/fix-audit2 branch November 3, 2026 13:16
@guijacquemet
guijacquemet restored the claude/fix-audit2 branch November 3, 2026 13:16
guijacquemet pushed a commit that referenced this pull request Nov 3, 2026
Q66 confirmed by the owner (3 November 2026); Q73's follow-up decided: neither a
stated reshape nor a reorder in v0.4, kept in issue #144; Q74 records the
tie-break and the consumers checked. The changelog gains a section for rc5 (PRs
#139, #140, #142, #143) with migration notes: mounts refused, position_id
required in v2, micrometre conversion within 1 ULP, --verify-content, and the
tie-break. The migration guide gets the same.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
@guijacquemet
guijacquemet deleted the claude/fix-audit2 branch November 14, 2026 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants