Reject a zero or non-finite ModelPixelScale on GeoTIFF read (#3331)#3332
Open
brendancol wants to merge 2 commits into
Open
Reject a zero or non-finite ModelPixelScale on GeoTIFF read (#3331)#3332brendancol wants to merge 2 commits into
brendancol wants to merge 2 commits into
Conversation
The direct-TIFF read path built coordinate arrays as arange(N) * pixel_width + origin straight from the ModelPixelScale (or ModelTransformation diagonal) with no finite-nonzero check. A zero pixel size collapsed the whole axis onto the origin; a NaN / Inf one filled it with NaN / Inf. Either way the read returned a degenerate raster with no error, so a downstream spatial op ran on coordinates that did not describe the data. The VRT read path already rejected a zero res_x / res_y (VRTUnsupportedError) and the writer rejected a zero-step coord axis (NonUniformCoordsError); this brings the direct-TIFF read path in line. Add DegeneratePixelSizeError (a GeoTIFFAmbiguousMetadataError subclass) and a _check_finite_nonzero_pixel_size guard in _extract_transform, covering the scale-only, ModelTiepoint + ModelPixelScale, and ModelTransformation paths. The guard sits in the shared geo-extraction path, so all four backends (numpy / dask / gpu / dask+gpu) reject the file identically. Closes #3331
brendancol
commented
Jun 15, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review: Reject a zero or non-finite ModelPixelScale on GeoTIFF read
Read all five changed files in full on the head branch.
Blockers
None.
Suggestions
None.
Nits
_geotags.py_check_finite_nonzero_pixel_size: for theModelPixelScalepath the caller passessx, sy, but the storedpixel_heightis-sy, so the message namespixel_heightwhile the tag value issy. For the zero case the sign matches, and for NaN / Inf the sign is immaterial, so the message is still accurate. Not worth changing.
What looks good
- The guard lives in the single shared
_extract_transform, so every backend (numpy / dask / gpu / dask+gpu) rejects the file identically. Confirmed by reading the call graph and running all four locally against a degenerate fixture. - All three georeferenced return paths are covered (scale-only, tiepoint + scale, ModelTransformation diagonal). The unit-scale tiepoint fallback (literal 1.0) and the
allow_rotatedno-georef path are left alone, which is correct. - A NaN ModelTransformation diagonal still ends up rejected: the rotation-tolerance check short-circuits on NaN (
x > NaNis False) and falls through to the new guard. Thenan_width/nan_heighttransformation cases pin this. DegeneratePixelSizeErrorsubclassesGeoTIFFAmbiguousMetadataError(and thereforeValueError), so existingexcept ValueErrorcallers keep catching it. Exported from the package and added to the safe-IO error table next to its siblings.- Tests cover zero / NaN / +-Inf on all three paths, the subclass contract, a positive control, and an end-to-end
open_geotiffrejection.
Checklist
- Behaviour matches the sibling guards (VRT
VRTUnsupportedError, writerNonUniformCoordsError) - All four backends reject consistently (shared extraction path)
- NaN / Inf handling correct, including the NaN-diagonal fall-through
- Edge cases covered (zero, NaN, +-Inf x both axes x three tag paths)
- No premature materialization (validation runs at tag-parse time, before pixel decode)
- Benchmark not needed (validation guard, not a compute path)
- Docs updated (safe-IO error table)
- Docstrings present on the new error class and helper
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 #3331.
_geotags._extract_transformreadsx = scale[0]/sy = scale[1](and theModelTransformationdiagonalm[0]/m[5]) with no finite-nonzero check, thencoords_from_pixel_geometrybuiltarange(N) * pixel_width + origin. A zero pixel size collapsed the axis onto the origin; a NaN / Inf one filled it with NaN / Inf. The read returned a degenerate raster with no error.The VRT read path already raises
VRTUnsupportedErrorfor a zerores_x/res_y, and the writer raisesNonUniformCoordsErrorfor a zero-step coord axis. This adds the matching rejection on the direct-TIFF read path.DegeneratePixelSizeError(subclass ofGeoTIFFAmbiguousMetadataError, soexcept ValueErrorkeeps catching it), exported fromxrspatial.geotiffand documented in the safe-IO error table._check_finite_nonzero_pixel_sizeguard in_extract_transformcovering the scale-only,ModelTiepoint+ModelPixelScale, andModelTransformationpaths. The unit-scale tiepoint-only fallback (always 1.0) is untouched, and the rotatedallow_rotatedpath (which returns no-georef) is untouched.Backend coverage: the guard sits in the shared geo-extraction path, so numpy, dask, gpu, and dask+gpu all reject the file identically. Verified all four locally on a degenerate fixture (CUDA available on this host).
Test plan:
test_degenerate_pixel_size_3331.py: zero / NaN / Inf on all three extraction paths, the subclass contract, positive controls, and an end-to-endopen_geotiffrejection.