geotiff: reject bool nodata in to_geotiff (#1911)#1917
Merged
Conversation
to_geotiff(..., nodata=True) was accepted without validation, and build_geo_tags then wrote GDAL_NODATA as the literal string "True". On read, that string cannot be parsed as numeric, so open_geotiff silently returned a DataArray with empty attrs and no mask. bool is a subclass of int in Python, so the typo slipped past every downstream isinstance(nodata, (int, float)) guard. Reject bool and np.bool_ at the to_geotiff entry point with a clear TypeError, matching the pattern already used in _reader.py and _vrt.py. Add the same check inside build_geo_tags so callers that bypass to_geotiff cannot reintroduce the bug. Fixes #1911.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes #1911 by preventing to_geotiff(..., nodata=True/False) (and np.bool_) from being accepted and written as the non-numeric GDAL_NODATA string "True"/"False", which previously broke nodata round-trips silently.
Changes:
- Add an early
TypeErrorguard into_geotiffrejectingbool/np.bool_fornodata. - Add the same defensive guard in
build_geo_tagsto protect callers that bypassto_geotiff. - Add regression tests covering bool rejection and numeric/None nodata behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
xrspatial/geotiff/_writers/eager.py |
Rejects bool/np.bool_ nodata at the public writer entry point with a clear TypeError. |
xrspatial/geotiff/_geotags.py |
Adds a belt-and-braces rejection of bool/np.bool_ before emitting TAG_GDAL_NODATA. |
xrspatial/geotiff/tests/test_nodata_bool_rejection_1911.py |
New regression coverage for bool rejection and numeric/None nodata behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [True, False, np.bool_(True), np.bool_(False)], | ||
| ) | ||
| def test_build_geo_tags_rejects_bool_nodata(bad): | ||
| """The lower-level builder also rejects bool, in case ``to_geotiff`` is bypassed.""" |
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
Fixes #1911.
to_geotiff(..., nodata=True)was accepted without validation, andbuild_geo_tagsthen wrote GDAL_NODATA as the literal string"True". The reader cannot parse that as numeric, soopen_geotiffsilently returned a DataArray with empty attrs and no mask.boolis a subclass ofintin Python, so the typo slipped past every downstreamisinstance(nodata, (int, float))guard.This PR rejects
boolandnp.bool_at theto_geotiffentry point with a clearTypeError, matching the pattern already used in_reader.pyand_vrt.py. The same check is added insidebuild_geo_tagsso callers that bypassto_geotiffcannot reintroduce the bug.Files
xrspatial/geotiff/_writers/eager.py: bool / np.bool_ guard at the top ofto_geotiffxrspatial/geotiff/_geotags.py: same guard insidebuild_geo_tags(belt-and-braces; uses a local numpy import to avoid adding a new top-level dep)xrspatial/geotiff/tests/test_nodata_bool_rejection_1911.py: new test fileTest plan
to_geotiff(..., nodata=True)raisesTypeErrorto_geotiff(..., nodata=False)raisesTypeErrorto_geotiff(..., nodata=np.bool_(True))raisesTypeErrorto_geotiff(..., nodata=0)still worksto_geotiff(..., nodata=0.0)still worksto_geotiff(..., nodata=-9999)still worksto_geotiff(..., nodata=None)still works (no GDAL_NODATA tag, nonodataattr on read)open_geotiffbuild_geo_tags(..., nodata=True)raisesTypeError