geotiff: reject bool nodata at write_vrt entry point (#1921)#1990
Merged
Conversation
write_vrt(nodata=True) used to write <NoDataValue>True</NoDataValue> into the VRT XML. bool is a subclass of int, so the typo slipped past the isinstance(nodata, (int, float)) guard and str(True) ended up in the XML. No reader parses "True" as numeric, so the round-trip silently dropped the sentinel. Fix: - Public write_vrt wrapper routes nodata through _validate_nodata_arg, matching the to_geotiff / write_geotiff_gpu surface. - Internal _vrt.write_vrt adds an explicit isinstance(bool, np.bool_) guard as defense-in-depth for direct callers and any future split of the wrapper. - Tests: flip the xfail(strict=True) parametrized bool-rejection cases to plain passes, drop the "current buggy behaviour" pin, and add a direct-internal-call test so the duplicate guard does not bit-rot.
Address review feedback on PR #1990: - Parametrize ``test_write_vrt_internal_rejects_bool_nodata`` over ``[True, False, np.bool_(True), np.bool_(False)]`` so a future refactor that narrows the internal guard to just ``bool`` (dropping ``np.bool_``) surfaces in the test, not in user code. - Update ``test_write_geotiff_gpu_rejects_bool_nodata`` docstring to name the proximate cause (``_validate_nodata_arg`` added by #1973) rather than the deeper ``build_geo_tags`` guard.
brendancol
added a commit
to brendancol/xarray-spatial
that referenced
this pull request
May 16, 2026
Self-review pass on PR xarray-contrib#1994: - _stamp_nodata_pixels: reject bool sentinels explicitly so a manifest entry like nodata: true can't slip a 1 into the raster. Matches the write-side gate from xarray-contrib#1990. - test_fixture_is_a_valid_tiff: tighten size budget from 4 KB to 2 KB (largest fixture today is 1402 bytes), so silent bloat trips the regression rather than drifting toward the documented limit. - test_int_sentinel_round_trips_through_rasterio: also assert src.nodata is not NaN, since rasterio reports nodata as a float. - manifest description: clarify that the masked-data path is reachable once Phase 3 backend wiring lands, not in this PR.
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
1 participant
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
Fixes #1921.
write_vrt(nodata=True)used to write<NoDataValue>True</NoDataValue>into the VRT XML and silently lose the sentinel on round-trip.boolis a subclass ofint, so the typo slipped past theisinstance(nodata, (int, float))guard andstr(True)ended up in the file.write_vrtwrapper (_writers/vrt.py) now routesnodatathrough_validate_nodata_arg, matching theto_geotiffandwrite_geotiff_gpusurface._vrt.write_vrtadds an explicitisinstance(nodata, (bool, np.bool_))check as defense-in-depth for direct callers and any future split of the wrapper.xfail(strict=True)parametrized bool-rejection cases pinned by PR geotiff: backend-parity tests for bool nodata rejection (#1921) #1924 flip to plain passes, the<NoDataValue>True</NoDataValue>"current buggy behaviour" pin is replaced with a direct-internal-call test so the duplicate guard cannot bit-rot, and the module docstring is updated.Test plan
pytest xrspatial/geotiff/tests/test_write_vrt_bool_nodata_1921.py-- 17 passed (was 13 passed + 4 xfailed)pytest xrspatial/geotiff/tests/ -k "vrt or nodata"-- 775 passed (one pre-existing unrelated failure intest_size_param_validation_gpu_vrt_1776fails the same way onmain)pytest xrspatial/geotiff/tests/test_nodata_bool_rejection_1911.py xrspatial/geotiff/tests/test_nodata_validation_1973.py-- 44 passed (sibling-writer parity intact)