geotiff: cover mask_nodata=False on GPU and VRT readers (#2052)#2102
Merged
Conversation
mask_nodata was wired through open_geotiff, read_geotiff_gpu, read_geotiff_dask, and read_vrt in #2052, but test_mask_nodata_kwarg only hit the eager-numpy and dask+numpy paths. The pure-GPU mask gating at _backends/gpu.py:709, the dask+GPU forwarding at _backends/gpu.py:991, the eager VRT gating at _backends/vrt.py:320, and the chunked VRT builder at _backends/vrt.py:408/588 had no direct coverage. A regression silently re-promoting integer rasters to float64 would slip past every existing sweep. 19 new tests, all passing on a GPU host. The fixture writes a tiled deflate-compressed uint16 so the pure nvCOMP decode path runs instead of the CPU-fallback piggyback. Covers GPU eager, dask+GPU, VRT eager, chunked VRT, dispatcher thread-through for each, and cross-backend bit-exact parity under mask_nodata=False. Mutation against gpu.py:709 flips 4 GPU tests red. Mutation against the eager VRT branch flips 4 VRT tests red. Tests only; no source changes.
brendancol
commented
May 19, 2026
Contributor
Author
brendancol
left a comment
There was a problem hiding this comment.
PR Review: geotiff: cover mask_nodata=False on GPU and VRT readers (#2052)
Blockers
None.
Suggestions
- The dask+GPU coverage is structurally real but may not exercise the GDS chunked builder in environments where kvikio is unavailable.
read_geotiff_gpu(chunks=...)first probes_gds_chunk_path_available(xrspatial/geotiff/_backends/gpu.py:775) and falls back to the CPU-decode + cupy upload path when kvikio is missing or itsCuFileattribute is unavailable. The test environment shows the warning_try_kvikio_read_tiles fell back to None (AttributeError: module 'kvikio' has no attribute 'CuFile'), so the dask+GPU tests in this PR flow throughread_geotiff_dask+map_blocks(cupy.asarray)(the kwarg is forwarded at gpu.py:991) rather than_read_geotiff_gpu_chunked_gds(gpu.py:1016). The kwarg forwarding still gets coverage on the CPU+upload branch, but a regression that dropsmask_nodatafrom the GDS chunked builder alone would not be caught here. Worth a comment in the test module flagging the dual-path nature so a future reader does not assume GDS coverage from these tests, or a kvikio-available CI lane to actually pin the GDS path.
Nits
- Module docstring at xrspatial/geotiff/tests/test_mask_nodata_gpu_vrt_2052.py:10-15 cites concrete line numbers (gpu.py:709, gpu.py:991, gpu.py:1016, vrt.py:320, vrt.py:408, vrt.py:588). These match HEAD now but will drift the moment an unrelated edit shifts those files. Symbol or function-name references (
_read_geotiff_gpu,_read_geotiff_gpu_chunked_gds, the eager / chunked VRT builders) would stay accurate without manual upkeep. Not blocking — the current refs are correct. - The uint16 4x4 sentinel array is declared twice (lines 80-86 and 103-109). A small shared constant would remove the duplication. Trivial.
What looks good
- Mutation evidence is concrete: gpu.py:709 and the eager VRT branch each have a documented 4-test red flip in the PR body, so the assertions actually pin the gating, not just the surrounding plumbing.
- Tests use bit-exact
np.testing.assert_array_equalagainst a known fixture instead of an approximation, which is the right strength for an integer-preservation contract. - Default-promotion tests (e.g. test_read_geotiff_gpu_default_mask_nodata_true_still_promotes at line 137, the chunked counterpart at line 201, VRT counterparts at line 249 and 297) pin both sides of the kwarg so a
mask_nodatadefault flip in either direction is caught. - Cross-backend parity tests (eager-numpy ↔ dask-numpy, eager-numpy ↔ eager-GPU, eager-numpy ↔ dask-GPU, eager-numpy ↔ eager-VRT) lock down semantic equivalence under the opt-out, not just per-backend behaviour.
- Direct entry-point tests (
read_geotiff_gpu,read_geotiff_dask,read_vrt) catch dispatcher-bypass regressions that theopen_geotiffthread-through tests would not, and vice versa. - Tiled + deflate fixture means the GPU eager path goes through nvCOMP decode rather than the CPU-fallback piggyback.
- Tests-only; zero production changes, so the risk surface is constrained to test correctness.
attrs['nodata'] == 0assertion at line 133 / line 246 catches the orthogonal regression wheremask_nodata=Falsedrops the sentinel from attrs entirely.
Checklist
- Algorithm matches reference/paper — N/A (coverage PR).
- All implemented backends produce consistent results — cross-backend parity tests confirm.
- NaN handling is correct — default-promotion tests verify the float64+NaN baseline using
np.isnan/cupy.isnan. - Edge cases are covered by tests — fixture intentionally has 0 sentinel positions inside a uint16 raster (the original bug shape).
- Dask chunk boundaries handled correctly —
chunks=2against a 4x4 raster forces multiple chunks, and.compute()parity holds. - No premature materialization or unnecessary copies — N/A (tests-only).
- Benchmark exists or is not needed — not needed.
- README feature matrix updated (if applicable) — not needed.
- Docstrings present and accurate — module docstring is informative; per-test docstrings are clear about intent.
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.
Summary
Coverage matrix
Mutation evidence
if nodata is not None and mask_nodata->if nodata is not None and True: flips 4 GPU tests red (eager-GPU preserves uint16, dispatcher thread-through, dtype=uint16 with opt-out, eager vs GPU parity).if mask_nodata->if True(eager VRT branch): flips 4 VRT tests red (eager VRT preserves uint16, dispatcher thread-through, dtype=uint16 with opt-out, eager vs VRT parity).Notes
Tests only; no source changes. Closes the Cat 1 HIGH backend-coverage gap flagged by pass 17 of the test-coverage sweep on the geotiff module.
Test plan