Fix _coords_to_transform for 3D (y,x,band) DataArrays (#1643)#1648
Merged
Conversation
_coords_to_transform read y/x coords via dims[-2:] which on a 3D (y, x, band) DataArray picked (x, band) instead of (y, x). to_geotiff and write_geotiff_gpu silently emitted a wrong GeoTransform on the fallback path when attrs['transform'] was absent (the round-tripped file used the band axis spacing as pixel_width). The helper now skips any trailing/leading dim named band/bands/channel and uses the two remaining spatial dims. 2D inputs and 3D (band, y, x) inputs are both handled.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes incorrect GeoTransform inference for 3D GeoTIFF-oriented xr.DataArrays by making _coords_to_transform select spatial y/x dims even when a band-like dim is present (e.g. (y, x, band) / (band, y, x)), preventing silently wrong transforms in to_geotiff / write_geotiff_gpu fallback paths.
Changes:
- Update
_coords_to_transformto ignore band-like dims (band/bands/channel) when inferring the transform for 3D arrays. - Add regression tests covering
_coords_to_transformbehavior and CPU/GPU round-trip parity for 3D inputs withoutattrs['transform'].
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
xrspatial/geotiff/__init__.py |
Adjusts _coords_to_transform to choose the two non-band dims for 3D arrays and documents the rationale. |
xrspatial/geotiff/tests/test_coords_to_transform_3d_1643.py |
Adds regression coverage for direct helper behavior and end-to-end GeoTIFF round-trips for 3D layouts (including GPU path). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+12
to
+14
| import os | ||
| import tempfile | ||
|
|
Comment on lines
+21
to
+27
| try: | ||
| import cupy # noqa: F401 | ||
| HAS_CUPY = True | ||
| except ImportError: | ||
| HAS_CUPY = False | ||
|
|
||
|
|
Comment on lines
+47
to
+64
| def test_coords_to_transform_yxband_returns_yx_spacing(): | ||
| """3D (y, x, band) picks y/x spacing rather than (x, band) spacing.""" | ||
| da = _make_geo_da_3d(('y', 'x', 'band')) | ||
| gt = _coords_to_transform(da) | ||
| # y spacing = (200 - 100) / 9, x spacing = (700 - 500) / 19 | ||
| assert gt is not None | ||
| np.testing.assert_allclose(gt.pixel_width, (700.0 - 500.0) / 19) | ||
| np.testing.assert_allclose(gt.pixel_height, (200.0 - 100.0) / 9) | ||
|
|
||
|
|
||
| def test_coords_to_transform_bandyx_returns_yx_spacing(): | ||
| """3D (band, y, x) also returns the y/x transform.""" | ||
| da = _make_geo_da_3d(('band', 'y', 'x')) | ||
| gt = _coords_to_transform(da) | ||
| assert gt is not None | ||
| np.testing.assert_allclose(gt.pixel_width, (700.0 - 500.0) / 19) | ||
| np.testing.assert_allclose(gt.pixel_height, (200.0 - 100.0) / 9) | ||
|
|
Comment on lines
+196
to
+203
| skips any trailing/leading dim named ``band`` / ``bands`` / ``channel`` | ||
| so a ``(y, x, band)`` or ``(band, y, x)`` DataArray returns the y/x | ||
| transform rather than picking up the band axis spacing as a pixel | ||
| size. ``to_geotiff`` itself remaps ``(band, y, x)`` arrays to | ||
| ``(y, x, band)`` before writing pixel bytes, but it calls | ||
| :func:`_coords_to_transform` against the original DataArray, so the | ||
| helper must handle both layouts to keep the geo-transform consistent | ||
| with the file's coord arrays. See issue #1643. |
Comment on lines
+205
to
+213
| _BAND_DIM_NAMES = ('band', 'bands', 'channel') | ||
| if da.ndim == 3: | ||
| # Drop the band-like dim and keep the two spatial dims in their | ||
| # original (y, x) order. Position-based fallback covers the case | ||
| # where none of the dims are named like a band axis. | ||
| spatial = tuple(d for d in da.dims if d not in _BAND_DIM_NAMES) | ||
| if len(spatial) == 2: | ||
| ydim, xdim = spatial[0], spatial[1] | ||
| else: |
- Lift _BAND_DIM_NAMES to module scope and reuse at the three (band,y,x) remap sites in __init__.py to avoid drift between _coords_to_transform and the writer paths. - Reword _coords_to_transform docstring: filter is position-independent, not trailing/leading. - Drop unused os/tempfile imports from the regression test. - Replace `import cupy` guard with the repo's standard _gpu_available() pattern that also checks `cupy.cuda.is_available()` and swallows non-ImportError import failures. - Add parametrized helper coverage for 'bands' and 'channel' dim names.
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 #1643.
_coords_to_transformlooked up y/x coords viadims[-2]/dims[-1]. On a 3D(y, x, band)DataArray that pickedxandbandinstead ofyandx, soto_geotiffandwrite_geotiff_gpusilently emitted a wrong GeoTransform whenever the fallback path ran (i.e.attrs['transform']was absent). The bug surfaced as a round-tripped file withpixel_width=1.0(the band axis spacing) and pixel coords offset by ~hundreds of metres on real rasters.The fix detects the band-like dim (
'band','bands','channel') and uses the two remaining spatial dims. 2D arrays keep their originaldims[-2:]behaviour. Both(y, x, band)and(band, y, x)3D layouts now resolve to the y/x transform.Test plan
xrspatial/geotiff/tests/test_coords_to_transform_3d_1643.pycover:(y, x, band)and(band, y, x)and 2D inputsto_geotiff+open_geotiffround-trip parity between 2D and 3Dwrite_geotiff_gpu) round-trip on CUDA hostsTestPalettefailures unrelated to this PR).