Fix geotiff metadata propagation gaps (#1580, #1582)#1585
Merged
Conversation
Two metadata-propagation defects flagged by the geotiff sweep: * #1580 -- write_geotiff_gpu treated arr.shape[:2] as (height, width) regardless of dim names, so a rioxarray-style (band, y, x) CuPy DataArray was written with the band axis stored as image width. Mirror the CPU writer's moveaxis remap when dims[0] is band/bands/ channel so both backends produce the same file for the same input. * #1582 -- to_geotiff (and the GPU writer it dispatches to) only consulted attrs['nodata'] when extracting the NoData sentinel, silently dropping rioxarray's attrs['nodatavals'] and CF-style attrs['_FillValue']. Add a _resolve_nodata_attr helper that walks those aliases in priority order (nodata -> nodatavals -> _FillValue) so a rioxarray-sourced DataArray round-trips with its sentinel intact. xrspatial's own nodata key still wins when both are set. Tests cover band-first dim ordering across all three accepted band dim names, byte-for-byte CPU/GPU parity, alias resolution priority, NaN-in-nodatavals (no GDAL_NODATA tag emitted), and the GPU writer honouring the same aliases.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes two GeoTIFF metadata/shape propagation defects in the xrspatial.geotiff writer stack to improve parity between CPU (to_geotiff) and GPU (write_geotiff_gpu) code paths, especially for rioxarray- and CF-style inputs.
Changes:
- Add
_resolve_nodata_attr()to resolve NoData fromattrs['nodata'],attrs['nodatavals'], orattrs['_FillValue'], and use it in both CPU and GPU write paths. - Fix GPU writer handling of band-first
(band, y, x)DataArrays by remapping to(y, x, band)before computing GeoTIFF dimensions. - Add regression tests covering nodata alias resolution and GPU band-first layout handling.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
xrspatial/geotiff/__init__.py |
Adds shared nodata-attr resolution and aligns GPU writer band-dimension handling with CPU behavior. |
xrspatial/geotiff/tests/test_nodata_attr_aliases_1582.py |
Regression tests ensuring nodatavals / _FillValue are propagated to the GeoTIFF nodata tag (CPU + GPU when available). |
xrspatial/geotiff/tests/test_gpu_writer_band_first_1580.py |
Regression tests ensuring GPU writer correctly handles (band, y, x) layout and matches CPU output. |
.claude/sweep-metadata-state.csv |
Records sweep state for geotiff module issues #1580 and #1582. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+664
to
+670
| first band's value; bands with mixed sentinels are uncommon and | ||
| would need an explicit ``nodata=`` argument anyway. | ||
| * ``attrs['_FillValue']`` -- CF-style xarray pipelines. | ||
|
|
||
| Returns ``None`` when none of the keys carry a usable value. NaN / | ||
| None entries in ``nodatavals`` are skipped rather than treated as | ||
| a sentinel (NaN means "the float NaN is the sentinel", which is |
Docstring said "Uses the first band's value" but the implementation returns the first usable entry, skipping None / non-numeric / NaN slots. Reword so callers don't assume only index 0 is consulted.
Contributor
Author
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
Closes #1580 and #1582 -- two metadata-propagation defects flagged by
the geotiff sweep.
write_geotiff_gputreatedarr.shape[:2]as(height, width)regardless of dim names, so a rioxarray-style
(band, y, x)CuPyDataArray was written with the band axis stored as image width and
the real width stored as samples-per-pixel. The CPU writer already
remaps
(band, y, x) -> (y, x, band)whendata.dims[0]isband/bands/channel; the GPU writer now mirrors that step so both
backends produce the same file for the same input.
to_geotiff(and the GPU writer it dispatches to) only consultedattrs['nodata']when extracting the NoData sentinel, silentlydropping rioxarray's
attrs['nodatavals']and CF-styleattrs['_FillValue']. A new_resolve_nodata_attrhelper walksthose aliases in priority order (
nodata->nodatavals->_FillValue). xrspatial's ownnodatakey still wins when bothare present, preserving the existing intra-library round-trip.
Test plan
pytest xrspatial/geotiff/tests/test_gpu_writer_band_first_1580.py-- 6 passedpytest xrspatial/geotiff/tests/test_nodata_attr_aliases_1582.py-- 11 passedpytest xrspatial/geotiff/tests/test_gpu_writer_attrs_1563.py xrspatial/geotiff/tests/test_attrs_parity_1548.py-- no regressionsxrspatial/geotiff/tests/-- only pre-existing matplotlibTestPalette::test_xrs_plot_*failures remain (unrelated tothis branch; reproducible on
main)