resample: reject irregular/non-monotonic spatial coordinates#2667
Conversation
Dedupe duplicate module rows (last-write-wins by last_inspected) and collapse multi-line notes to single physical lines. The notes had embedded newlines, which the merge=union .gitattributes strategy splits record-by-record, corrupting the file into a 156-column phantom row on parallel-agent appends. One line per record keeps union merges safe.
brendancol
left a comment
There was a problem hiding this comment.
PR Review: resample: reject irregular/non-monotonic spatial coordinates
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
- The even-spacing tolerance combines
rtol=1e-5withatol=abs(step) * 1e-5(xrspatial/resample.py:236), so both terms scale withstep. It works, but theatolterm is redundant:np.allclosealready usesatol + rtol * |b|withb = step, sortolalone scales by|step|. Either drop theatolterm or keep it as a deliberate small floor. Behavior is correct as written.
Nits (optional improvements)
_validate_monotonic_regular_coordsre-runs the coord checks once per band on 3D inputs, becauseresample()recurses per-band and revalidates. Harmless (the coords are identical and the check is cheap), but a one-line note would make the redundancy explicit.
What looks good
- The guard runs before any geometry math or backend dispatch, so the bad-geometry path is closed at the source.
- A descending y-axis (north-up rasters) is correctly accepted via
all(diffs > 0) or all(diffs < 0). - Missing-coords and length-1 axes are skipped rather than erroring, matching the existing fall-through behavior.
- Coords are numpy-backed in xarray regardless of data backend, so one check covers all four backends. The tests parametrize over every available backend anyway.
- Error messages name the offending dimension and state the contract.
Checklist
- Behavior matches the documented contract (regular monotonic rasters only)
- All implemented backends produce consistent results (coord check is backend-independent)
- NaN handling unaffected (validation is on coords, not data)
- Edge cases covered by tests (missing coords, single-pixel axis, float jitter, descending y)
- Dask chunk boundaries: not applicable, no chunked compute added
- No premature materialization (coords are already eager numpy)
- Benchmark: not needed, validation-only change
- README feature matrix: not applicable, no new function or backend change
- Docstring updated (Raises section)
brendancol
left a comment
There was a problem hiding this comment.
Follow-up review (after commit 2df9efc)
Both items from the first pass are resolved:
- Spacing tolerance: the
atol=abs(step) * 1e-5term is gone. The check now usesnp.allclose(diffs, step, rtol=1e-5, atol=0.0), so a singlertolcontrols the tolerance and it tracks the coordinate magnitude. The float-jitter test still passes. - Per-band re-validation: documented in the helper docstring as a cheap, harmless repeat on identical coords.
No new findings. Tests: 240 passed across the resample suite (numpy + dask backends; cupy backends gated by availability).
Checklist
- Behavior matches the documented contract
- Backend-independent coord check, parametrized tests
- NaN handling unaffected
- Edge cases covered
- No premature materialization
- Docstring updated
Use has_cuda_and_cupy()/has_dask_array() boolean predicates from xrspatial.utils instead of the skipif MarkDecorators from general_checks. Calling a MarkDecorator is always truthy, so the cupy backends were added unconditionally and the cupy tests ran on CI (no cupy installed), failing at import cupy.
brendancol
left a comment
There was a problem hiding this comment.
CI fix (commit 46bbcc9)
The first CI run failed: the new test file gated its cupy backends with cuda_and_cupy_available() / dask_array_available() imported from general_checks, but those names are pytest.mark.skipif MarkDecorators, not boolean predicates. Calling a MarkDecorator is always truthy, so the cupy and dask+cupy backends were added unconditionally and those parametrized cases ran on CI (where cupy is not installed), failing at import cupy. It passed locally only because cupy is present here, so the cases ran on real hardware and masked the bug.
Fix: switch to the boolean predicates has_cuda_and_cupy() and has_dask_array() from xrspatial.utils (the convention used in test_reproject.py). With cupy absent, BACKENDS is now ['numpy', 'dask+numpy'] and no cupy cases are collected. Verified by simulating cupy-absent locally.
Post-fix CI: pytest (ubuntu/windows/macos 3.14), pytest-cog-validator (3.12), pytest-geotiff-corpus, and Label PRs all green. Resample suite: 364 passed locally.
Closes #2663
Problem
resampleassumes a regular, monotonic raster but never checks for one.target_resolutionderives the input resolution fromcalc_res()(full-extent(max - min) / (n - 1)), while the output coordinates are rebuilt from first/last neighbour spacing in_new_coords. On an irregular grid those two views of resolution disagree, so the output width and coordinate range stop matching the input. Withx=[0, 1, 4]andtarget_resolution=1.0the output had width 6 and x coords[0, 1, 2, 3, 4, 5], spilling past the input range[0, 4].Change
_validate_monotonic_regular_coords, called at the top ofresample(), which rejects spatial coordinates that are non-monotonic or unevenly spaced with a clearValueError.Backends
Coordinate validation reads numpy-backed coords regardless of the data backend, so the guard behaves identically across numpy, cupy, dask+numpy, and dask+cupy. Tests parametrize over every available backend.
Test plan