Skip to content

Make corpus determinism check toolchain-agnostic (#2299)#2377

Merged
brendancol merged 2 commits into
mainfrom
issue-2299
May 25, 2026
Merged

Make corpus determinism check toolchain-agnostic (#2299)#2377
brendancol merged 2 commits into
mainfrom
issue-2299

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Summary

  • test_corpus_determinism.py picks the comparison method per fixture. Byte-md5 for fixtures that are stable across GDAL / libjpeg builds; a semantic rasterio-read comparison (dtype, shape, transform, CRS, nodata, overview chain, pixels) for cog: true and compression: jpeg cells where the encoder output is toolchain-coupled.
  • Lossy cells (tolerance.lossy: true in the manifest, currently the JPEG-YCbCr fixture) skip the pixel-equality check.
  • The pytest-geotiff-corpus workflow drops its --ignore=test_corpus_determinism.py flag now that the test is no longer toolchain-coupled.

Closes #2299.

Test plan

  • pytest xrspatial/geotiff/tests/golden_corpus/test_corpus_determinism.py: 33 passed, 1 skipped.
  • pytest xrspatial/geotiff/tests/golden_corpus/: 94 passed, 1 skipped.
  • Manual negative test: doctored copies that flip a pixel are rejected by _assert_semantic_equal.
  • conda-forge pytest-geotiff-corpus workflow goes green on PR with the COG and JPEG fixtures in scope.

@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 25, 2026
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

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

PR Review: Make corpus determinism check toolchain-agnostic

Blockers

None.

Suggestions

  • _assert_semantic_equal only compares base-IFD pixels even when both files have an overview chain (test_corpus_determinism.py:143-154). For the COG fixture, an overview-pixel regression in the generator would slip past: the decimation factor list matches but the overview pixels never get read. Consider iterating the overview levels for lossless cells with rasterio.open(path, OVERVIEW_LEVEL=lvl) and comparing each level's pixel array. _oracle.py already uses that pattern in compare_to_oracle.
  • Lossy JPEG cells skip pixel comparison entirely (line 147-148). A pixel-content regression in the generator's JPEG branch (wrong input, wrong band order before encode) would not be caught here. A coarse band-wise mean check within a loose tolerance would close the gap without re-introducing libjpeg coupling.

Nits

  • _is_encoder_sensitive, _assert_semantic_equal, and _nodata_equal have no direct unit tests. The drift-detection paths were verified manually in the PR; a tiny test that builds a doctored copy with a flipped pixel and asserts _assert_semantic_equal raises would lock the negative path in.
  • The tmp-dir name in regenerated_dir is still "regen_corpus_1930" (line 178). Harmless but misleading now that the test's scope was extended in #2299.

What looks good

  • Dispatch is driven from manifest fields (cog, compression, tolerance.lossy), not hard-coded fixture ids, so new encoder-sensitive cells inherit the right comparison mode automatically.
  • The semantic check covers dtype, shape, transform, CRS, nodata (NaN-aware via _nodata_equal), overview decimation chain, and lossless pixels.
  • The workflow comment explains why the --ignore is gone instead of leaving a silent loosening.

Checklist

  • Algorithm matches reference: no algorithm; test-infra change
  • All implemented backends produce consistent results: rasterio-only
  • NaN handling is correct: _nodata_equal + equal_nan=True
  • Edge cases: missing-fixture skip preserved
  • Overview-level pixels exercised: not yet (see suggestion)
  • Dask chunk boundaries: n/a
  • No premature materialization: n/a
  • Benchmark: n/a
  • README feature matrix: n/a
  • Docstrings present and accurate

COG and JPEG fixtures fall back to a semantic rasterio-read
comparison instead of byte-md5; the rest still md5-compare. The
conda-forge workflow drops its --ignore now that the test handles
encoder drift on its own.
…2299)

- Lossless cells now also compare every overview level's pixel array,
  closing the gap where the COG fixture's pyramid could drift unnoticed.
- Lossy (JPEG) cells gain a coarse per-band mean tolerance so a
  content regression in the JPEG branch is still detected.
- Add direct negative-path tests that doctor a fixture and assert
  _assert_semantic_equal rejects it.
- Rename the regenerated_dir tmp prefix to reflect the test's scope.
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

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

Follow-up review

All four findings from the prior review pass were addressed in f5aa2141:

  • Suggestion 1 (overview-level pixels): _assert_semantic_equal now iterates rasterio.open(..., OVERVIEW_LEVEL=lvl) for every level the file declares and compares each level's pixel array (lossless cells). See _assert_overview_pixels_exact at test_corpus_determinism.py:194.
  • Suggestion 2 (lossy mean tolerance): _assert_pixels_close_lossy at test_corpus_determinism.py:209 computes per-band means and rejects drift above _LOSSY_PIXEL_MEAN_TOL = 4.0 (0-255 scale).
  • Nit 1 (drift unit tests): two new tests cover the negative path: test_semantic_equal_rejects_lossless_pixel_drift and test_semantic_equal_rejects_lossy_mean_drift (test_corpus_determinism.py:357 and :371).
  • Nit 2 (tmp-dir rename): tmp_path_factory.mktemp("regen_corpus_determinism") at test_corpus_determinism.py:256.

The branch was rebased onto origin/main after the follow-up so the PR diff is now scoped to the two #2299 files; force-push was clean.

Local run: pytest xrspatial/geotiff/tests/golden_corpus/: 96 passed, 1 skipped.

No new blockers, suggestions, or nits from this pass.

@brendancol brendancol merged commit 4c9e11e into main May 25, 2026
4 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance PR touches performance-sensitive code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

geotiff corpus: determinism test is toolchain-coupled (conda-forge GDAL differs from fixture-build env)

1 participant