Skip to content

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

Open
brendancol wants to merge 1 commit into
mainfrom
issue-1972
Open

geotiff: reject (y, x, time) 3D writer inputs (#1972)#1982
brendancol wants to merge 1 commit into
mainfrom
issue-1972

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #1972.

Summary

  • _validate_3d_writer_dims now refuses 3D writer inputs whose trailing dim is a known temporal name (time, t, date, datetime, times, dates). The mirror case (time, y, x) was already rejected by the existing layout check; this closes the asymmetry.
  • The error message points the caller at isel, a reduction, or a rename to band if the temporal axis is genuinely the band axis.
  • The bare (y, x, *) fallback for raw numpy callers building band-last arrays stays in place for genuinely unknown trailing dim names.

Test plan

  • New xrspatial/geotiff/tests/test_temporal_3d_writer_rejection_1972.py covers each temporal alias, each y/x alias, the band-leading and band-trailing accept cases, the regression on (time, y, x), and the writer entry point round trip.
  • Full pytest xrspatial/geotiff/tests/ — same 8 pre-existing failures as main, none related.

@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 15, 2026
(time, y, x) was already caught by _validate_3d_writer_dims (issue
#1812), but the symmetric (y, x, time) slipped through the
(y, x, *) band-position fallback and was silently written as a
3-band TIFF. Round-tripping a temporal stack therefore produced a
file that looked like a multiband raster.

Add _TIME_DIM_NAMES in _runtime.py (time / t / date / datetime /
times / dates) and check the trailing dim against it in
_validate_3d_writer_dims. Known temporal names raise with a message
suggesting isel / mean / rename-to-band; the (y, x, *) fallback
for genuinely unknown trailing dim names stays in place so raw
numpy callers building band-last arrays are not bounced.
@brendancol
Copy link
Copy Markdown
Contributor Author

PR Review (self-review)

Suggestions

  • Case-sensitivity gap. _TIME_DIM_NAMES = ('time', 't', 'date', 'datetime', 'times', 'dates') is matched with in (case-sensitive). Verified locally: ('y', 'x', 'TIME') still slips through and writes a 3-band TIFF, because 'TIME' is not in the tuple and the (y, x, *) fallback fires. Many real datasets use 'TIME' or 'Time' (CF allows either). Either store lowercase variants in the tuple, or compare with d2.lower() in _TIME_DIM_NAMES. The existing _Y_DIM_NAMES / _X_DIM_NAMES are also case-sensitive, but the failure mode there is loud rejection rather than silent corruption — temporal trailing dim is the bigger risk and warrants case-insensitive matching.

Nits

  • The error message references data.isel({d2}=0) literally. For d2='time' this renders as data.isel(time=0), which is correct. For aliases like d2='dates' it renders as data.isel(dates=0), which is also correct because that's the actual dim name. Fine as-is, but worth a passing read in case an alias was added that wasn't a real xarray dim name.
  • (time, y, x) still falls through to the generic "ambiguous dims" error from the existing validator. The new dedicated temporal message is only attached to the trailing-dim case. Could symmetrise by checking d0 against _TIME_DIM_NAMES too and emitting the same friendly message, though the asymmetry is functional (the leading case was already rejected).

What looks good

  • The fix lands in _validate_3d_writer_dims, so the eager, GPU, and .vrt-dispatch writer entry points all pick it up without further changes.
  • The (y, x, *) fallback for genuinely unknown dim names (raw numpy callers building band-last arrays) stays intact.
  • Error message points the caller at concrete fixes: isel, a reduction, or rename to band.
  • Tests cover each temporal alias, each y/x alias paired with time, both accept cases (band-leading / band-trailing), and the regression on (time, y, x).

Checklist

  • Algorithm matches reference/paper — not applicable
  • All implemented backends covered — validator is shared across writer entry points
  • NaN handling — not applicable
  • Edge cases covered — uppercase / mixed-case temporal names slip through (see suggestion)
  • Dask chunk boundaries — not applicable
  • No premature materialisation
  • No new benchmark needed
  • Docstrings present and accurate

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.

geotiff: (y, x, time) DataArrays silently treated as multiband rasters

1 participant