Skip to content

geotiff: GPU + dask+GPU coverage for predictor=3 + integer reject (#1933)#1951

Open
brendancol wants to merge 2 commits into
xarray-contrib:mainfrom
brendancol:deep-sweep-test-coverage-geotiff-2026-05-15-1778858256-02
Open

geotiff: GPU + dask+GPU coverage for predictor=3 + integer reject (#1933)#1951
brendancol wants to merge 2 commits into
xarray-contrib:mainfrom
brendancol:deep-sweep-test-coverage-geotiff-2026-05-15-1778858256-02

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Summary

Closes a Cat 1 HIGH backend-parity gap for issue #1933 (_validate_predictor_sample_format). The eager numpy and dask paths are covered by test_predictor3_int_dtype_1933.py; the cupy and dask+cupy paths had no targeted tests.

The validator is invoked at two GPU sites:

  • _backends/gpu.py:443 -- tiled eager GPU read
  • _backends/gpu.py:999 -- GDS chunked GPU read

A regression dropping either call would let malformed predictor=3 + integer SampleFormat tiled files decode silently to garbage bytes on GPU.

9 new tests, all passing on a CUDA host:

  • read_geotiff_gpu on stripped + tiled malformed files
  • open_geotiff(gpu=True) and open_geotiff(chunks=, gpu=True) dispatchers
  • read_geotiff_gpu(chunks=) on stripped + tiled
  • Legitimate predictor=3 + float32 tiled file still round-trips on GPU (eager + dask+GPU)
  • Error-message parity between GPU and eager paths

Mutation against the tiled GPU validator at gpu.py:443 flipped the tiled-raises test red. Mutation against the GDS chunked validator at gpu.py:999 flipped the chunked-tiled + chunked-dispatcher tests red.

This is a deep-sweep test-coverage PR. No source files were modified.

Test plan

  • pytest xrspatial/geotiff/tests/test_predictor3_int_dtype_gpu_1933.py (9 passed)
  • pytest xrspatial/geotiff/tests/test_predictor3_int_dtype_1933.py xrspatial/geotiff/tests/test_predictor3_int_dtype_gpu_1933.py (19 passed)
  • Mutation: drop _validate_predictor_sample_format at gpu.py:443 -> test_gpu_eager_tiled_raises fails
  • Mutation: drop _validate_predictor_sample_format at gpu.py:999 -> test_read_geotiff_gpu_chunked_tiled_raises + test_open_geotiff_chunks_gpu_dispatcher_raises fail

Copilot AI review requested due to automatic review settings May 15, 2026 15:28
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 15, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@brendancol brendancol force-pushed the deep-sweep-test-coverage-geotiff-2026-05-15-1778858256-02 branch from 86fa8f3 to 4711485 Compare May 15, 2026 15:42
@brendancol
Copy link
Copy Markdown
Contributor Author

PR Review: geotiff: GPU + dask+GPU coverage for predictor=3 + integer reject (#1933)

Blockers (must fix before merge)

  • None

Suggestions (should fix, not blocking)

  • test_read_geotiff_gpu_chunked_tiled_raises (test_predictor3_int_dtype_gpu_1933.py:213) accepts the rejection from either the GDS validator at gpu.py:999 or the CPU dask fallback. On a host without KvikIO the GDS validator at line 999 is never exercised, so the mutation-kill claim in the PR description only holds on KvikIO-enabled CI. Consider gating the test on kvikio availability (pytest.importorskip("kvikio")) or adding a sibling test that does so, otherwise the line-999 call site is not actually pinned by the suite in the general case.
  • test_predictor3_float32_gpu_round_trip and test_predictor3_float32_dask_gpu_round_trip (lines 245, 258) use tifffile.imwrite for the legitimate-file case while the malformed cases use the in-tree _assemble_standard_layout. Building the valid file through the same in-tree writer would remove the tifffile/imagecodecs skip dependency and keep the round-trip contract honest against the writer the rest of the suite exercises.

Nits (optional improvements)

  • _build_predictor3_uint32_stripped_tiff (line 70) duplicates the helper in test_predictor3_int_dtype_1933.py:42. A shared helper in tests/conftest.py would keep the two modules from drifting if the malformed-file shape needs to change.
  • Module docstring (lines 11-18) hard-codes gpu.py:443 and gpu.py:999. These will rot when the file is edited. A symbolic reference (the function name) would survive line renumbering.
  • test_gpu_error_message_matches_eager (line 285) uses == on the full message. If the eager site ever appends file path or band info, both messages will diverge and the test fails for a non-regression. A startswith or shared substring check is more robust.

What looks good

  • Tiled vs stripped split is well motivated: the stripped path falls through to the CPU validator, so the tiled fixture is what actually exercises gpu.py:443.
  • Both dispatchers (open_geotiff(gpu=True) and open_geotiff(chunks=, gpu=True)) are covered alongside the direct read_geotiff_gpu entrypoints.
  • Tmp filenames are unique per test, no collisions.
  • Skip guard via cupy.cuda.is_available() is correct and keeps the file inert on non-GPU CI.
  • Test-only PR, no source changes, matches the deep-sweep coverage charter.

Checklist

  • Algorithm matches reference/paper (validator behaviour, no algorithm change)
  • All implemented backends produce consistent results (parity test asserts identical message)
  • NaN handling is correct (n/a, integer rejection)
  • Edge cases are covered by tests (tiled + stripped + dispatcher + chunked)
  • Dask chunk boundaries handled correctly (chunked tests use chunks= smaller than the array)
  • No premature materialization or unnecessary copies
  • Benchmark exists or is not needed (not needed, error path)
  • README feature matrix updated (if applicable) (n/a)
  • Docstrings present and accurate (modulo the line-number rot nit)

brendancol added a commit to brendancol/xarray-spatial that referenced this pull request May 15, 2026
brendancol added a commit to brendancol/xarray-spatial that referenced this pull request May 15, 2026
@brendancol brendancol force-pushed the deep-sweep-test-coverage-geotiff-2026-05-15-1778858256-02 branch from 2c26804 to 1e34c92 Compare May 15, 2026 17:29
@brendancol
Copy link
Copy Markdown
Contributor Author

PR Review: geotiff: GPU + dask+GPU coverage for predictor=3 + integer reject (#1933) (re-review after rebase)

Rebased cleanly onto origin/main at 1e34c92, picking up the Windows fix from 9ce0e60. Local run of xrspatial/geotiff/tests/test_predictor3_int_dtype_gpu_1933.py passes (8 passed, 1 skipped on CPU-only host where libkvikio is unavailable).

Blockers

  • None

Suggestions

  • None

Nits

  • The pytest.importorskip("kvikio") call raises a PytestDeprecationWarning on hosts where the kvikio module is importable but its shared library is missing. Passing exc_type=ImportError would silence the warning ahead of pytest 9.1, but this is cosmetic.

What looks good

  • Test coverage now spans cupy and dask+cupy paths for the predictor=3 + integer SampleFormat reject behavior added in geotiff: predictor=3 + integer dtype silently misdecoded on read #1933.
  • The in-tree writer keeps the round-trip tests self-contained and avoids relying on any external writer behavior.
  • kvikio gating via importorskip keeps the GPU-only tests honest on CPU CI runners.

Checklist

  • Rebase clean on origin/main
  • Tests pass locally
  • No code changes beyond rebase
  • Pushed with --force-with-lease to fork

…rray-contrib#1933)

xarray-contrib#1933 added _validate_predictor_sample_format and wired it into every
IFD-read site, including two GPU sites that had no targeted tests:

- _backends/gpu.py:443 -- tiled eager GPU validator
- _backends/gpu.py:999 -- GDS chunked GPU validator

The eager and dask paths are covered by test_predictor3_int_dtype_1933.
A regression dropping either GPU validator call would let malformed
predictor=3 + integer tiled files decode silently to garbage bytes on
GPU and ship under existing CI.

Adds 9 tests, all passing on a CUDA host:
- read_geotiff_gpu on stripped + tiled malformed files
- open_geotiff(gpu=True) and open_geotiff(chunks=, gpu=True) dispatchers
- read_geotiff_gpu(chunks=) on stripped + tiled
- legitimate predictor=3 + float32 tiled file still round-trips on GPU
  (eager + dask+GPU)
- error-message parity between GPU and eager paths

Mutation against the tiled GPU validator at gpu.py:443 flipped the
tiled-raises test red; mutation against the GDS chunked validator at
gpu.py:999 flipped the chunked-tiled + chunked-dispatcher tests red.
@brendancol
Copy link
Copy Markdown
Contributor Author

Rebased onto current main at 4dec5b6 to pick up the Python 3.14 CI matrix limit (b1579f8).

@brendancol brendancol force-pushed the deep-sweep-test-coverage-geotiff-2026-05-15-1778858256-02 branch from 1e34c92 to 4dec5b6 Compare May 15, 2026 17:53
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.

2 participants