Add external COG writer compliance test suite (#2292)#2297
Merged
brendancol merged 2 commits intoMay 22, 2026
Conversation
New file xrspatial/geotiff/tests/test_cog_writer_compliance.py opens to_geotiff(cog=True) output through rasterio and walks the writer across a matrix of stable codecs (none, deflate, lzw, zstd, packbits), dtypes (uint16, float32), band counts (1 and 3), nodata flavours (sentinel + NaN), pixel-is-area vs pixel-is-point, and explicit-list vs auto-generated overview pyramids. Each row asserts base pixels are byte-exact at full resolution, overview decimations match, CRS / transform / nodata / dtype / band count survive the round-trip, the file is tiled, and IFDs sit before image data. Optional rio-cogeo / GDAL validate_cloud_optimized_geotiff check skips cleanly when neither is installed. Closes xarray-contrib#2292.
brendancol
commented
May 22, 2026
Contributor
Author
brendancol
left a comment
There was a problem hiding this comment.
Review: external COG writer compliance suite (#2292)
Tests-only PR. The backend dispatch / Dask / CuPy / performance sections of the standard review do not apply here. Focused review on test quality, coverage of the stated matrix, and a few minor cleanups.
Blockers
None.
Suggestions
- Missing overview pixel-value assertion. The issue asks for "Overview pixels match expected within tolerance", but the suite only checks overview dims and decimation factors.
test_overviews_explicit_levelsreadsOVERVIEW_LEVEL=lvland asserts the shape; it never compares the overview pixels to a reference. Ameanoverview is deterministic from the base array, so we can compute the expected 2x2 block-mean ourselves and assert allclose. Worth adding at least one row that pins overview values. - No "intended to be stable" resampling-mode coverage. Issue mentions "Overview resampling modes that are intended to be stable" as part of the matrix. The existing assertions all rely on the default
mean. One small param test over['mean', 'nearest']would close that gap. test_nodata_sentinel_survivesfor float32 only round-trips one sentinel value (-9999.0). That is fine for the spec, but a positive sentinel (e.g.np.finfo(np.float32).max) would exercise the path where the sentinel collides with valid data range. Optional.
Nits
xrspatial/geotiff/tests/test_cog_writer_compliance.py:252:assert src.transform.is_identity is False—is Falseon a property is brittle (works today, breaks if the attr ever returns a truthy non-bool).assert not src.transform.is_identityis cleaner.xrspatial/geotiff/tests/test_cog_writer_compliance.py:195: local variablewarningsshadows the stdlibwarningsmodule. Rename to_warnsto mirror the rio-cogeo branch above it.xrspatial/geotiff/tests/test_cog_writer_compliance.py:126:_pick_sentinelhas a signed-int branch (iinfo(dt).min) that is never reached -- the DTYPES list only hasuint16andfloat32. Either drop the dead branch or document it as a future-proof.xrspatial/geotiff/tests/test_cog_writer_compliance.py:396: the power-of-two check(o & (o - 1)) == 0is correct given theo >= 2guard but the intent is not obvious at a glance. A short comment ("bitwise: power of two iff o & (o-1) == 0") would help future readers.- The big parametrized matrix is named
test_codec_dtype_bands_roundtrip. The two assertions that fire after the read (src.transform.is_identity,src.crs.to_epsg() == 4326) cover CRS/transform survival. Worth a one-line comment at the top of the test naming each contract it covers, so a future reader does not need to scan the body.
What looks good
- Black-box via rasterio is the right framing -- the in-process round-trip tests already exist elsewhere, and this suite catches the cross-reader interop class of bugs the parent issue cares about.
- Optional rio-cogeo / GDAL validator path with a clean
pytest.skipwhen neither is installed, exactly as the issue spec asks for. - COG layout assertion (
_assert_ifds_before_data) is reused across the explicit / auto / layout tests rather than duplicated. - Temporary filenames carry the
2292_issue prefix per project conventions for parallel-safe paths. - 28 passed locally, 1 skipped (the validator path). No
xfailrows needed -- nothing in the matrix surfaced a writer bug.
Checklist
- Matrix matches the issue scope (codecs, dtypes, bands, nodata, georef, overviews)
- Optional validators handled with
pytest.skip - No production code modified
- Temp paths use a unique
2292_prefix - Overview pixel values (not just dims) compared against expected -- suggestion above
- At least one overview-resampling mode beyond the
meandefault exercised -- suggestion above
- Add ``test_overview_pixels_match_expected`` over ``['mean', 'nearest']`` resampling, comparing the level-1 overview against a hand-computed 2x decimation. Closes the "overview pixels match expected" gap from the issue spec and exercises a second resampling mode. - Add docstring to ``test_codec_dtype_bands_roundtrip`` enumerating the contracts each row asserts. - Comment the bitwise power-of-two check in ``test_overviews_auto_generated``. - Replace ``is False`` on ``src.transform.is_identity`` with ``not``. - Rename shadowing ``warnings`` local in ``_try_cog_validate``. - Document the unreachable signed-int branch in ``_pick_sentinel``.
brendancol
commented
May 22, 2026
Contributor
Author
brendancol
left a comment
There was a problem hiding this comment.
Follow-up review (#2292)
All items from the previous review are addressed:
- Overview pixel-value comparison: new
test_overview_pixels_match_expectedruns bothmeanandnearestagainst a hand-computed 2x decimation, with a 1 ULP tolerance documented in the comment. test_codec_dtype_bands_roundtripnow opens with a docstring listing the contracts each row asserts.test_overviews_auto_generatedcarries an inline note explaining the bitwise power-of-two check.is Falseonsrc.transform.is_identityreplaced withnot.warningslocal in_try_cog_validaterenamed to_warns._pick_sentineldocuments the unreachable signed-int branch.
Local run: 30 passed, 1 skipped (the external validator step skips when neither rio-cogeo nor osgeo_utils is installed).
No new blockers, suggestions, or nits. PR is ready from a review standpoint pending CI and the merge-with-main check.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2292. Part of #2286 (COG readiness/stability rollout, sub-PR 2/6).
Summary
Adds
xrspatial/geotiff/tests/test_cog_writer_compliance.py, anexternal-interop suite that opens
to_geotiff(..., cog=True)outputthrough rasterio and walks the writer across a stable-codec matrix.
No production code changed — this is a tests-only PR.
The matrix covers:
none,deflate,lzw,zstd,packbits.uint16andfloat32.[2, 4, 8]list and auto-generated pyramids.Each row asserts base pixels are byte-exact at full resolution (stable
codecs are lossless), overview factors and per-level dims match, the
file is tiled, IFDs sit before image data, and CRS / transform /
nodata / dtype / band count all survive the round-trip.
The suite also runs the optional rio-cogeo
cog_validate/GDAL
validate_cloud_optimized_geotiffcheck, andpytest.skipscleanly when neither package is installed.
Test plan
pytest xrspatial/geotiff/tests/test_cog_writer_compliance.py— 28 passed, 1 skipped locally