Skip to content

geotiff: reject ambiguous 3D writer inputs (#1812)#1820

Merged
brendancol merged 3 commits into
mainfrom
deep-sweep-metadata-geotiff-2026-05-13-af588e03
May 13, 2026
Merged

geotiff: reject ambiguous 3D writer inputs (#1812)#1820
brendancol merged 3 commits into
mainfrom
deep-sweep-metadata-geotiff-2026-05-13-af588e03

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Summary

  • Fixes to_geotiff: silent data corruption on 3D input with non-whitelisted leading dim #1812: to_geotiff and write_geotiff_gpu used to silently corrupt data when a 3D DataArray's leading dim was not in _BAND_DIM_NAMES = ('band', 'bands', 'channel'). The moveaxis that puts (band, y, x) into the on-disk (y, x, band) layout was skipped, the writer kept the leading axis as the spatial y axis, and the round-trip produced a TIFF with silently swapped axes.
  • Reject ambiguous 3D layouts at all three writer entry points (eager to_geotiff, dask streaming, write_geotiff_gpu) via a shared _validate_3d_writer_dims helper. Accepted layouts: (band, y, x) or (y, x, band) plus band aliases bands/channel and spatial aliases lat/lon/latitude/longitude/row/col. Anything else raises ValueError with an actionable fix message.
  • Surfaced by the 2026-05-13 metadata propagation sweep.

Repro (pre-fix)

arr = np.zeros((2, 4, 5), dtype=np.uint8)
arr[0] = 1; arr[1] = 2
da = xr.DataArray(arr, dims=('time', 'y', 'x'), attrs={'crs': 'EPSG:4326'})
to_geotiff(da, '/tmp/bad.tif', crs=4326)
out = open_geotiff('/tmp/bad.tif')
out.shape                  # (2, 4, 5)  wrong; should be (4, 5, 2)
out.values[:, :, 0].sum()  # 12         wrong; should be arr[0].sum() == 20

After the fix the same call raises ValueError with a message naming the offending dims and pointing to either transpose('y', 'x', 'time') or rename to one of _BAND_DIM_NAMES.

Test plan

  • New regression file xrspatial/geotiff/tests/test_to_geotiff_3d_dim_validation_1812.py (19 cases)
    • Original repro now raises ValueError
    • Eager / dask-streaming / GPU paths all reject ambiguous layouts
    • Happy paths (band, y, x), (bands, y, x), (channel, y, x), (y, x, band), (lat, lon, band), (row, col, channel), (band, lat, lon) round-trip with correct per-band sums
    • 2D (y, x) still works untouched
    • Error message contains the offending dims, both accepted layouts, the issue number, and a remediation verb (transpose / rename)
  • Full xrspatial/geotiff/tests/ suite passes (2311 passed, 5 skipped; 11 deselected pre-existing failures unrelated to this change -- matplotlib path deepcopy recursion on Python 3.14 and test_predictor2_big_endian_gpu_1517 / test_size_param_validation_gpu_vrt_1776::test_tile_size_positive_works / test_vrt_dstrect_resample_cap_1737::test_dstrect_at_cap_succeeds baseline failures, all confirmed pre-existing via git stash).

to_geotiff and write_geotiff_gpu used to silently mishandle 3D
DataArrays whose leading dim was not in _BAND_DIM_NAMES = ('band',
'bands', 'channel'). The moveaxis that puts (band, y, x) into the
on-disk (y, x, band) layout was skipped, the writer kept the leading
axis as the spatial y axis, and the round-trip produced a TIFF with
silently swapped axes -- on read-back, out[:, :, 0].sum() !=
arr[0].sum().

Reject ambiguous 3D layouts at all three writer entry points (eager
to_geotiff, dask streaming, write_geotiff_gpu) via the shared
_validate_3d_writer_dims helper. Accepted layouts: (band, y, x) or
(y, x, band) with band-name aliases bands/channel and spatial-name
aliases lat/lon/latitude/longitude/row/col. Anything else raises
ValueError with an actionable message (rename the non-spatial dim
or transpose).

Surfaced by the 2026-05-13 metadata propagation sweep.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 13, 2026
@brendancol brendancol requested a review from Copilot May 13, 2026 16:38
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Closes #1812: the GeoTIFF writers (to_geotiff eager, to_geotiff dask-streaming, and write_geotiff_gpu) used to silently corrupt 3D DataArrays whose leading dim was not in _BAND_DIM_NAMES because the moveaxis to (y, x, band) was skipped, leaving the leading axis to be treated as y on disk. The PR introduces a shared _validate_3d_writer_dims helper that rejects any 3D layout that is not (band, y, x) or (y, x, band) (with accepted aliases) and raises ValueError with an actionable remediation message.

Changes:

  • Add _validate_3d_writer_dims helper plus _Y_DIM_NAMES / _X_DIM_NAMES alias lists in xrspatial/geotiff/__init__.py, and call it from all three 3D writer entry points (eager, dask streaming, GPU).
  • Update docstrings of to_geotiff and write_geotiff_gpu with a Raises entry describing the new validation.
  • Add a regression test module covering the original repro, the three writer paths, accepted layouts (including aliases), 2D pass-through, GPU happy paths, and the error-message contract.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
xrspatial/geotiff/init.py Adds the _validate_3d_writer_dims gate and wires it into the eager, dask-streaming, and GPU writer entry points; updates docstrings.
xrspatial/geotiff/tests/test_to_geotiff_3d_dim_validation_1812.py New regression tests for ambiguous-layout rejection, happy-path round-trips, 2D pass-through, and error-message content (CPU + GPU).
.claude/sweep-metadata-state.csv Sweep state row updated to record the 2026-05-13 audit and the new HIGH finding (#1812).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +42 to +51
# Inputs that *must* raise. Each tuple is (dims, shape).
_AMBIGUOUS_3D_INPUTS = [
pytest.param(("time", "y", "x"), (2, 4, 5), id="time-y-x"),
pytest.param(("z", "y", "x"), (2, 4, 5), id="z-y-x"),
pytest.param(("band", "lat", "lon"), (2, 4, 5), id="band-lat-lon"), # ok via alias
pytest.param(("y", "x", "depth"), (4, 5, 2), id="y-x-depth"), # accepted: spatial-first
pytest.param(("foo", "bar", "baz"), (2, 4, 5), id="foo-bar-baz"),
]


PR #1803 forwarded the caller's max_pixels to read_to_array inside
read_vrt's source loop so a tiny VRT output cannot force a huge source
decode (#1796). The output-window check at the source read enforces that
correctly. A separate per-tile dimension check at the same call sites
also consumed the caller's max_pixels, so a caller setting max_pixels as
an output budget (e.g. 10_000) failed the per-tile sanity check on any
normal source whose default tile size is 256x256 (= 65_536 pixels).

Use MAX_PIXELS_DEFAULT for the per-tile dim check at the two call sites
in _read_tiles (local) and _read_tiles_cog_http (HTTP). The output-window
check at the same functions continues to enforce the user-supplied
max_pixels, preserving the #1796 protection.
@brendancol brendancol merged commit e21bf3c into main May 13, 2026
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance PR touches performance-sensitive code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

to_geotiff: silent data corruption on 3D input with non-whitelisted leading dim

2 participants