Skip to content

Reject non-finite/fractional int GDAL_NODATA on read (#2441)#2442

Merged
brendancol merged 2 commits into
mainfrom
issue-2441
May 26, 2026
Merged

Reject non-finite/fractional int GDAL_NODATA on read (#2441)#2442
brendancol merged 2 commits into
mainfrom
issue-2441

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Summary

  • Adds InvalidIntegerNodataError (a GeoTIFFAmbiguousMetadataError subclass) and raises it from extract_geo_info when an integer-dtype source declares a non-finite (NaN, Inf, -Inf) or fractional (3.5) GDAL_NODATA value.
  • Adds allow_invalid_nodata: bool = False on open_geotiff, read_geotiff_dask, read_geotiff_gpu, and read_vrt. Threaded through _read_to_array, _read_cog_http, _read_geo_info, and the dask chunk tasks.
  • Updates test_nodata_nan_int_1774.py to exercise the legacy no-op behaviour via the opt-in. New default-rejection coverage lives in test_invalid_int_nodata_rejection_2441.py.
  • Flips the test_release_gate_negative_integer_nodata_float_promoted xfail.

Backend coverage

Numpy, cupy, dask+numpy, dask+cupy. All four read paths share the same validator via extract_geo_info, and the opt-in is threaded through every entry point, so the rejection contract is identical across backends.

Test plan

  • pytest xrspatial/geotiff/tests/test_invalid_int_nodata_rejection_2441.py (new default-rejection coverage: eager numpy, dask, GPU, plus float-source, finite-int, and opt-in regression guards)
  • pytest xrspatial/geotiff/tests/test_nodata_nan_int_1774.py (legacy no-op behaviour still works under the opt-in)
  • pytest xrspatial/geotiff/tests/release_gates/test_stable_features.py -m release_gate (xfail flipped to pass; other release-gate tests unaffected)
  • pytest xrspatial/geotiff (5802 passed, 5 xfailed, 0 failed)
  • pytest xrspatial (10666 passed, 5 xfailed; two failing *_dask_temp_cleanup tests are pre-existing flakes unrelated to this change)

Closes #2441.

Upgrade the silent no-op from #1774 to a typed
``InvalidIntegerNodataError`` at the read boundary so callers cannot
quietly mismask integer GeoTIFFs whose ``GDAL_NODATA`` is NaN, Inf,
or fractional. The legacy behaviour remains available via
``allow_invalid_nodata=True``, threaded through every public read
entry point. Flips the
``test_release_gate_negative_integer_nodata_float_promoted`` xfail.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 26, 2026
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

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

PR Review: Reject non-finite/fractional int GDAL_NODATA on read (#2441)

Blockers (must fix before merge)

None. Implementation is correct and tests pass across all exercised backends.

Suggestions (should fix, not blocking)

  • xrspatial/geotiff/_vrt.py:1964 (write_vrt / _compute_vrt_metadata): the VRT writer reads source-TIFF metadata via extract_geo_info(ifd, data, header.byte_order) without allow_invalid_nodata=True. After this PR, that call inherits the new default-rejection behaviour, so write_vrt(..., source_files=[<int.tif with GDAL_NODATA="nan">, ...]) will raise InvalidIntegerNodataError even though the writer is only reading source metadata to populate the VRT XML (no pixel masking). Either pass allow_invalid_nodata=True at this call site (the writer is not masking) or surface the new opt-in on write_vrt so callers can keep planning VRTs over quirky sources.

Nits (optional improvements)

  • xrspatial/geotiff/tests/test_invalid_int_nodata_rejection_2441.py: there is no explicit test for the dask+cupy combination (gpu=True, chunks=...). The validation fires at metadata parse before any chunking, so coverage is logically subsumed by the eager-GPU and eager-dask tests, but adding a one-line read_geotiff_gpu(path, chunks=2) rejection test would close the four-backend matrix explicitly.

What looks good

  • _validate_int_nodata_for_dtype (xrspatial/geotiff/_validation.py:599) is a clean leaf. It short-circuits on None, on float dtypes, and on the opt-in flag before doing any work.
  • The error message names the offending sentinel value, the kind (non-finite or fractional), the source dtype, the opt-in flag name, and cites release_gate_geotiff.rst. It satisfies every assertion in the release-gate test ('nodata', 'allow_invalid_nodata', 'non-finite'/'fractional', dtype name, contract hint).
  • InvalidIntegerNodataError correctly subclasses GeoTIFFAmbiguousMetadataError, so existing except GeoTIFFAmbiguousMetadataError callers keep catching it.
  • The opt-in threads through extract_geo_info, extract_geo_info_with_overview_inheritance, _read_to_array, _read_cog_http, _read_geo_info, _delayed_read_window, and the GPU chunked / eager-via-cpu helpers. _CANONICAL_ORDER in test_reader_kwarg_order_1935.py is updated to keep the kwarg-order contract intact.
  • Lazy import inside extract_geo_info avoids the _validation -> _coords -> _geotags circular import path. A short comment explains why.
  • test_nodata_nan_int_1774.py is updated rather than deleted, keeping the historical context while routing the legacy no-op behaviour through the new opt-in.

Checklist

  • Algorithm matches the cited release contract (#1774 follow-up, release_gate_geotiff)
  • All implemented backends (numpy / cupy / dask+numpy / dask+cupy) hit the same validator via extract_geo_info
  • Float-source NaN nodata stays unaffected (explicitly tested)
  • Finite in-range integer sentinels stay unaffected (explicitly tested)
  • Opt-in flag (allow_invalid_nodata=True) restores the legacy no-op (tested for eager + dask + GPU)
  • No premature materialization or unnecessary copies (validator is metadata-only)
  • Docstrings present and accurate on the new error class, the new validator, and the new opt-in kwarg on every public entry point
  • No README feature-matrix change needed (no new public function; behavioural upgrade only)

…2441)

- ``_compute_vrt_metadata`` now passes ``allow_invalid_nodata=True``
  when reading source-TIFF metadata. The writer only populates VRT
  XML; it does not decode or mask pixels, so the rejection contract
  does not apply at this site (the read-side default still rejects
  when the VRT is later opened).
- New ``test_read_geotiff_gpu_chunked_int_nodata_rejected_by_default``
  closes the four-backend matrix on the rejection contract.
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

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

Follow-up review

Both findings from the previous pass have been addressed in 881ea8b.

Suggestion 1 (VRT writer site) -- fixed

xrspatial/geotiff/_vrt.py:1964 (_compute_vrt_metadata) now passes allow_invalid_nodata=True when reading source-TIFF metadata. The writer only populates VRT XML; it does not decode or mask pixels, so the rejection contract does not apply at this call site. A reader that later opens the resulting VRT still hits the default rejection.

Nit 1 (dask+cupy explicit coverage) -- fixed

Added test_read_geotiff_gpu_chunked_int_nodata_rejected_by_default in test_invalid_int_nodata_rejection_2441.py. The four-backend rejection matrix is now explicit: eager numpy, eager dask, eager GPU, dask+cupy.

Verification

  • pytest xrspatial/geotiff -- 5803 passed, 5 xfailed, 0 failed (one new test added).
  • No new findings on a re-read of the changed files.

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.

GeoTIFF reader: upgrade non-finite/fractional int nodata from silent no-op to typed rejection (#1774 follow-up)

1 participant