Reject zero and non-finite SCALE/OFFSET under unpack=True; guard pack=True against divide-by-zero (#3104)#3129
Merged
Conversation
brendancol
commented
Jun 9, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review: Reject zero and non-finite SCALE/OFFSET under unpack=True; guard pack=True against divide-by-zero (#3104)
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
- The PR body says unpack is "rejected at the boundary on GPU and VRT paths". True for VRT, stale for GPU: #3075 added unpack on the GPU and dask+GPU read paths. Both route through
_extract_scale_offset(GPU eager via_finalize_eager_readin_attrs.py, dask+GPU via_backends/dask.py:434), so the fix already covers them. Confirmed with a live CUDA run:open_geotiff(path, unpack=True, gpu=True)and thechunks=2variant both raiseMalformedScaleOffsetErroron a SCALE=0 file, and a valid SCALE=2 file still unpacks. Update the PR body and add arequires_gpu-gated case totest_scale_zero_3104.pyso the GPU path stays pinned by CI on GPU hosts.
Nits (optional improvements)
- The module docstring of
test_scale_zero_3104.pynames only the eager and dask paths. Mention the GPU paths once the gated test lands.
What looks good
- The rejection lives in the one helper all read paths share, so there is no per-backend drift to maintain.
MalformedScaleOffsetErrorkeeps the #2992 contract (aValueErrorsubclass), so existingexcept ValueErrorcallers keep catching it.- Tests cover
-0.0(which compares equal to 0.0) and pin negative finite SCALE as still honoured, so legitimate inverted-scale files keep working. - The
_packguard names both attrs and the reason in its message, and the hand-edited-attrs route has its own tests.
Checklist
- Algorithm matches reference (n/a: validation change)
- Backends consistent: numpy/dask tested; GPU verified by hand, gated test requested above
- NaN handling correct (non-finite values rejected up front)
- Edge cases covered: 0, -0.0, nan, +/-inf, offset variants, no-unpack passthrough
- Dask chunk boundaries: n/a, rejection happens at metadata time before graph build
- No premature materialization (raises before any pixel work)
- Benchmark not needed
- README feature matrix not applicable
- Docstrings updated
brendancol
commented
Jun 9, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
Follow-up review of 85544b1 (GPU-gated tests added after the first pass):
- Both suggestions are addressed:
test_unpack_rejects_zero_scale_gpucovers GPU and dask+GPU through therequires_gpumarker (the suite's usable-CUDA probe, not a bare cupy import),test_valid_scale_still_unpacks_gpupins the non-rejection side, and the module docstring now names all four read paths. The PR body's backend-coverage paragraph was corrected too. - The
hasattr(arr, "get")guard in the valid-scale GPU test is right: small GPU reads can come back as host arrays, so an unconditional.get()would break. - Ran the file on this CUDA host: 28 passed. No new findings.
Disposition of the first-pass findings: both fixed in-PR, nothing deferred or dismissed.
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 #3104
_extract_scale_offsetnow raisesMalformedScaleOffsetErrorwhen SCALE is zero or non-finite, or when OFFSET is non-finite. This is the same fail-closed handling geotiff: reject malformed SCALE/OFFSET under mask_and_scale #2992 gave unparseable values: a SCALE of 0 collapses every pixel to the offset on read and has no inverse forpack=True._packrefuses a zero or non-finitescale_factor/ non-finiteadd_offset(reachable only via hand-edited attrs once the read side rejects them) instead of dividing by zero and writing a file where every pixel is the nodata sentinel.Backend coverage: the fix lives in the one metadata helper every unpack path resolves through. Eager numpy and GPU reads call it via
_finalize_eager_read; dask and dask+GPU call it in_backends/dask.py. The VRT path rejectsunpack=Trueat the boundary and is unaffected. GPU and dask+GPU rejection is covered byrequires_gpu-gated tests and was verified on a live CUDA host.Test plan:
xrspatial/geotiff/tests/read/test_scale_zero_3104.py: SCALE 0/-0.0/nan/inf/-inf and OFFSET nan/inf rejection on numpy and dask, GPU and dask+GPU rejection plus a valid-scale GPU unpack (gated onrequires_gpu), zero SCALE ignored without unpack, negative finite SCALE still honoured, pack guards for hand-edited attrs (28 cases on a CUDA host, 25 without)xrspatial/geotiff/tests/read+tests/write: 2352 passed, 5 skippedAlso carries the security-sweep state CSV update for the geotiff module (audit pass 2026-06-09).