geotiff: accept numpy integer CRS values at writers (#2082)#2085
Merged
Conversation
_validate_crs_arg already accepts numbers.Integral (so np.int32 / np.int64 / np.uint16 pass), but the writers gated EPSG assignment on isinstance(crs, int). Numpy integers failed that check and the value silently fell through with no EPSG and no crs_wkt written; read-back returned a DataArray with no CRS at all. Widen the three writer-side checks (eager.py:540, eager.py:871, gpu.py:349) to numbers.Integral and coerce to int(crs). Validator already rejects bool so the writer branch is safe from the bool subclass slip. The VRT writer path goes through _resolve_crs_to_wkt which already coerces, so no change there. Add regression tests that round-trip np.int32 / np.int64 / np.uint16 through to_geotiff (path + BytesIO), the VRT-tiled branch, and write_geotiff_gpu (skipped without cuda). Tests check that attrs['crs'] resolves to the expected EPSG, not just that bytes got written -- the pre-existing test_to_geotiff_accepts_numpy_int_epsg only asserted nbytes > 0, which passed even with no CRS in the file.
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
isinstance(crs, int)checks tonumbers.Integralso numpy integer scalars (np.int32,np.int64,np.uint16) hit the EPSG branch instead of falling through with no CRS written.to_geotiff(path + BytesIO), the VRT-tiled branch, andwrite_geotiff_gpu, checkingattrs['crs']resolves to the expected EPSG instead of just asserting bytes hit disk.Background
_validate_crs_argat_crs.py:87acceptsnumbers.Integral. The writer paths checkedisinstance(crs, int), which isFalsefor numpy integers. Soto_geotiff(..., crs=np.int64(4326))passed validation, fell through both writer branches, and produced a file with no EPSG and nocrs_wkt. Reading the file back returned a DataArray with no CRS, silent metadata loss.Three sites:
xrspatial/geotiff/_writers/eager.py:540(eagerto_geotiff)xrspatial/geotiff/_writers/eager.py:871(deprecated_write_vrt_tiled)xrspatial/geotiff/_writers/gpu.py:349(write_geotiff_gpu)The VRT writer path goes through
_resolve_crs_to_wkt, which already coercesnumbers.Integral, so no change there.The existing
test_to_geotiff_accepts_numpy_int_epsgonly assertedbuf.getbuffer().nbytes > 0. A file with no CRS still has bytes, so that test passed even with the bug in place. The new tests checkattrs['crs']after round-trip.Backend coverage
eager.pyeager.pyentry point, coveredgpu.py, regression test is cuda-gatedCloses #2082.
Test plan
pytest xrspatial/geotiff/tests/test_numpy_int_crs_2082.py-- 9 pass (GPU test skipped without cuda)pytest xrspatial/geotiff/tests/test_crs_arg_validation_1971.py xrspatial/geotiff/tests/test_conflicting_crs_write_1987.py-- 34 pass, no regressionspytest xrspatial/geotiff/tests/ -k "crs or write"-- 845 pass, 1 pre-existing failure intest_size_param_validation_gpu_vrt_1776unrelated to this change