geotiff: reject (y, x, time) 3D writer inputs (#1972)

xrspatial/geotiff/_runtime.py

@@ -62,6 +62,12 @@
 _Y_DIM_NAMES = ('y', 'lat', 'latitude', 'row')
 _X_DIM_NAMES = ('x', 'lon', 'longitude', 'col')
 
+# Temporal dim names. Used by the 3D writer validator (#1972) to refuse
+# ``(y, x, <temporal>)`` inputs that would otherwise be silently treated
+# as multiband rasters. CF / xarray conventions cover ``time`` and ``t``;
+# the rest match common upstream-pipeline aliases.
+_TIME_DIM_NAMES = ('time', 't', 'date', 'datetime', 'times', 'dates')
+
 
 class GeoTIFFFallbackWarning(UserWarning):
     """Warning emitted when a geotiff helper falls back to a slower path.

xrspatial/geotiff/_validation.py

@@ -14,7 +14,18 @@
 import numpy as np
 
 from ._coords import _BAND_DIM_NAMES
-from ._runtime import _X_DIM_NAMES, _Y_DIM_NAMES
+from ._runtime import _TIME_DIM_NAMES, _X_DIM_NAMES, _Y_DIM_NAMES
+
+
+def _is_temporal_dim_name(name) -> bool:
+    """Return True if ``name`` is a known temporal dim alias.
+
+    Compared case-insensitively against ``_TIME_DIM_NAMES`` so that
+    CF-style ``'TIME'`` / ``'Time'`` reach the friendly temporal error
+    in the 3D writer validator instead of slipping through the
+    ``(y, x, *)`` band-position fallback (#1972).
+    """
+    return isinstance(name, str) and name.lower() in _TIME_DIM_NAMES
 
 
 def _validate_3d_writer_dims(dims) -> None:
@@ -43,13 +54,41 @@ def _validate_3d_writer_dims(dims) -> None:
                   and d2 in _BAND_DIM_NAMES)
     if band_layout or yxb_layout:
         return
-    # Bare (y, x, *) or (*, y, x) where the third dim is unnamed but
-    # spatial -- the writer's old behaviour treats the non-spatial axis
-    # as bands. Accept that only when the unknown dim is in the band
-    # position (last), which matches how raw numpy callers typically
-    # build a band-last array.
+    # Bare (y, x, *) where the third dim is unnamed but spatial -- the
+    # writer's old behaviour treats the non-spatial axis as bands.
+    # Accept that only when the unknown dim is in the band position
+    # (last), which matches how raw numpy callers typically build a
+    # band-last array. Refuse known *temporal* dim names so a
+    # ``(y, x, time)`` stack is rejected with a clear error instead of
+    # silently being written as a 3-band TIFF (issue #1972). The
+    # mirror case ``(time, y, x)`` was already caught -- this closes
+    # the asymmetry.
     if d0 in _Y_DIM_NAMES and d1 in _X_DIM_NAMES:
+        if _is_temporal_dim_name(d2):
+            raise ValueError(
+                f"3D writer input has temporal trailing dim {d2!r} in dims "
+                f"{dims!r}. The writer would otherwise treat the time axis "
+                f"as bands and silently write a multiband TIFF. Select a "
+                f"single time slice (e.g. ``data.isel({d2}=0)``), reduce "
+                f"with a stat (``data.mean({d2!r})``), or rename to one of "
+                f"{_BAND_DIM_NAMES} if you really intend the temporal "
+                f"axis to round-trip as TIFF bands (issue #1972)."
+            )
         return
+    # Symmetrise the friendly temporal message for the leading-dim case
+    # ``(time, y, x)``. The generic ``ambiguous dims`` error below
+    # already rejects this layout, but the temporal-specific message
+    # tells the caller exactly how to fix it (#1972).
+    if _is_temporal_dim_name(d0) and d1 in _Y_DIM_NAMES and d2 in _X_DIM_NAMES:
+        raise ValueError(
+            f"3D writer input has temporal leading dim {d0!r} in dims "
+            f"{dims!r}. The writer would otherwise treat the time axis "
+            f"as bands and silently write a multiband TIFF. Select a "
+            f"single time slice (e.g. ``data.isel({d0}=0)``), reduce "
+            f"with a stat (``data.mean({d0!r})``), or rename to one of "
+            f"{_BAND_DIM_NAMES} if you really intend the temporal "
+            f"axis to round-trip as TIFF bands (issue #1972)."
+        )
     raise ValueError(
         f"3D writer input has ambiguous dims {dims!r}. Expected "
         f"(band, y, x) or (y, x, band); accepted band-dim aliases are "

xrspatial/geotiff/tests/test_temporal_3d_writer_rejection_1972.py

@@ -0,0 +1,121 @@
+"""Refuse ``(y, x, <time>)`` 3D writer inputs (#1972).
+
+The existing ``_validate_3d_writer_dims`` (issue #1812) rejected the
+``(time, y, x)`` case, but the symmetric ``(y, x, time)`` slipped
+through the ``(y, x, *)`` band-position fallback and was silently
+written as a 3-band TIFF. This locks the temporal-trailing case down
+across the eager, GPU (import-time-check only), and `.vrt` writer
+entry points.
+"""
+from __future__ import annotations
+
+import io
+
+import numpy as np
+import pytest
+import xarray as xr
+
+from xrspatial.geotiff import to_geotiff
+from xrspatial.geotiff._validation import _validate_3d_writer_dims
+
+
+@pytest.mark.parametrize(
+    "temporal",
+    ['time', 't', 'date', 'datetime', 'times', 'dates'],
+)
+def test_validate_3d_rejects_yx_temporal(temporal):
+    with pytest.raises(ValueError, match="temporal trailing dim"):
+        _validate_3d_writer_dims(('y', 'x', temporal))
+
+
+@pytest.mark.parametrize(
+    "temporal",
+    ['TIME', 'Time', 'TiMe', 'DATE', 'Datetime', 'DATES', 'T'],
+)
+def test_validate_3d_rejects_yx_temporal_case_insensitive(temporal):
+    # CF allows ``'TIME'`` / ``'Time'``; the lowercase _TIME_DIM_NAMES
+    # tuple must still match via case-insensitive comparison so the
+    # mixed-case stack does not slip through the (y, x, *) fallback and
+    # silently write a 3-band TIFF (#1972 self-review).
+    with pytest.raises(ValueError, match="temporal trailing dim"):
+        _validate_3d_writer_dims(('y', 'x', temporal))
+
+
+@pytest.mark.parametrize(
+    "yx",
+    [('y', 'x'), ('lat', 'lon'), ('latitude', 'longitude'), ('row', 'col')],
+)
+def test_validate_3d_rejects_yx_aliases_with_temporal(yx):
+    with pytest.raises(ValueError, match="temporal trailing dim"):
+        _validate_3d_writer_dims((yx[0], yx[1], 'time'))
+
+
+def test_validate_3d_still_accepts_yx_band():
+    _validate_3d_writer_dims(('y', 'x', 'band'))
+    _validate_3d_writer_dims(('band', 'y', 'x'))
+
+
+def test_validate_3d_still_accepts_unknown_trailing_dim():
+    # The (y, x, *) fallback for raw numpy callers stays in place for
+    # genuinely unknown dim names; only known temporal names trip.
+    _validate_3d_writer_dims(('y', 'x', 'channel'))
+    _validate_3d_writer_dims(('y', 'x', 'foo'))
+
+
+def test_validate_3d_still_rejects_time_y_x():
+    # Leading temporal dim was already rejected; the symmetrised path
+    # now emits the dedicated temporal message (#1972 self-review nit 2)
+    # instead of the generic "ambiguous dims" wording.
+    with pytest.raises(ValueError, match="temporal leading dim"):
+        _validate_3d_writer_dims(('time', 'y', 'x'))
+
+
+@pytest.mark.parametrize(
+    "temporal",
+    ['time', 'TIME', 'Time', 't', 'T', 'date', 'datetime', 'dates'],
+)
+def test_validate_3d_rejects_temporal_y_x_case_insensitive(temporal):
+    # Mirror the trailing-dim case-insensitive coverage for the leading
+    # temporal axis (#1972 self-review nit 2).
+    with pytest.raises(ValueError, match="temporal leading dim"):
+        _validate_3d_writer_dims((temporal, 'y', 'x'))
+
+
+def test_validate_3d_rejects_temporal_yx_alias_leading():
+    # Leading-dim friendly message should fire for y/x aliases too.
+    with pytest.raises(ValueError, match="temporal leading dim"):
+        _validate_3d_writer_dims(('time', 'lat', 'lon'))
+
+
+def test_validate_3d_still_rejects_other_ambiguous_leading():
+    # The symmetric temporal message must not swallow the generic
+    # ambiguous-dims path for non-temporal, non-band leading names.
+    with pytest.raises(ValueError, match="ambiguous dims"):
+        _validate_3d_writer_dims(('foo', 'y', 'x'))
+
+
+def test_to_geotiff_rejects_yxtime_stack():
+    da = xr.DataArray(
+        np.zeros((4, 4, 3), dtype=np.float32),
+        coords={'y': np.arange(4.0), 'x': np.arange(4.0),
+                'time': np.arange(3)},
+        dims=('y', 'x', 'time'),
+    )
+    buf = io.BytesIO()
+    with pytest.raises(ValueError, match="temporal trailing dim"):
+        to_geotiff(da, buf)
+
+
+def test_error_message_suggests_isel_and_band_rename():
+    da = xr.DataArray(
+        np.zeros((4, 4, 3), dtype=np.float32),
+        coords={'y': np.arange(4.0), 'x': np.arange(4.0),
+                'time': np.arange(3)},
+        dims=('y', 'x', 'time'),
+    )
+    buf = io.BytesIO()
+    with pytest.raises(ValueError) as excinfo:
+        to_geotiff(da, buf)
+    msg = str(excinfo.value)
+    assert "isel(time=0)" in msg
+    assert "band" in msg.lower()

xrspatial/geotiff/tests/test_to_geotiff_3d_dim_validation_1812.py

@@ -68,14 +68,19 @@ def _make_da(dims, shape, dtype=np.uint8, backend="numpy"):
 
 
 def test_repro_silent_corruption_now_raises(tmp_path):
-    """The original repro now raises a clear ValueError."""
+    """The original repro now raises a clear ValueError.
+
+    Post-#1972 the ``(time, y, x)`` layout produces the dedicated
+    temporal-leading-dim message rather than the generic ambiguous-dims
+    one, so accept either wording.
+    """
     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"})
     out_path = tmp_path / "tmp_1812_time_y_x.tif"
-    with pytest.raises(ValueError, match="ambiguous dims"):
+    with pytest.raises(ValueError, match="ambiguous dims|temporal leading dim"):
         to_geotiff(da, str(out_path), crs=4326)
 
 
@@ -88,7 +93,7 @@ def test_eager_rejects_ambiguous_3d(tmp_path, dims, shape):
     """Eager numpy path raises ValueError on ambiguous 3D dim names."""
     da = _make_da(dims, shape, backend="numpy")
     out_path = tmp_path / f"tmp_1812_eager_{'_'.join(dims)}.tif"
-    with pytest.raises(ValueError, match="ambiguous dims"):
+    with pytest.raises(ValueError, match="ambiguous dims|temporal leading dim"):
         to_geotiff(da, str(out_path), crs=4326)
 
 
@@ -101,7 +106,7 @@ def test_dask_streaming_rejects_ambiguous_3d(tmp_path, dims, shape):
     """Dask-streaming branch raises ValueError on ambiguous 3D dim names."""
     da = _make_da(dims, shape, backend="dask")
     out_path = tmp_path / f"tmp_1812_dask_{'_'.join(dims)}.tif"
-    with pytest.raises(ValueError, match="ambiguous dims"):
+    with pytest.raises(ValueError, match="ambiguous dims|temporal leading dim"):
         to_geotiff(da, str(out_path), crs=4326)
 
 
@@ -116,7 +121,7 @@ def test_gpu_writer_rejects_ambiguous_3d(tmp_path, dims, shape):
 
     da = _make_da(dims, shape, backend="cupy")
     out_path = tmp_path / f"tmp_1812_gpu_{'_'.join(dims)}.tif"
-    with pytest.raises(ValueError, match="ambiguous dims"):
+    with pytest.raises(ValueError, match="ambiguous dims|temporal leading dim"):
         write_geotiff_gpu(da, str(out_path), crs=4326)
 
 
@@ -179,16 +184,21 @@ def test_2d_still_works(tmp_path):
 
 
 def test_error_message_actionable(tmp_path):
-    """The ValueError message tells the caller how to fix the input."""
+    """The generic ValueError message tells the caller how to fix the input.
+
+    Uses a non-temporal leading dim so the dedicated #1972 temporal path
+    does not short-circuit, keeping the assertions on the generic
+    "(band, y, x)" / "(y, x, band)" / "#1812" wording intact.
+    """
     arr = np.zeros((2, 4, 5), dtype=np.uint8)
-    da = xr.DataArray(arr, dims=("time", "y", "x"),
+    da = xr.DataArray(arr, dims=("z", "y", "x"),
                       attrs={"crs": "EPSG:4326"})
     p = tmp_path / "tmp_1812_msg.tif"
     with pytest.raises(ValueError) as excinfo:
         to_geotiff(da, str(p), crs=4326)
     msg = str(excinfo.value)
     # Mentions the offending dim layout
-    assert "('time', 'y', 'x')" in msg
+    assert "('z', 'y', 'x')" in msg
     # Mentions the accepted alternatives
     assert "(band, y, x)" in msg
     assert "(y, x, band)" in msg