Skip to content

Warn when planar resolution averages irregular coordinates (#2766)#2784

Merged
brendancol merged 3 commits into
mainfrom
issue-2766
Jun 1, 2026
Merged

Warn when planar resolution averages irregular coordinates (#2766)#2784
brendancol merged 3 commits into
mainfrom
issue-2766

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2766

What changed

  • calc_res now checks whether each 1-D spatial coordinate is evenly spaced. When it isn't, it emits a UserWarning instead of quietly reducing the irregular grid to one averaged cell size, and points at attrs["res"] for an explicit override.
  • The averaged resolution value is unchanged, so regular grids behave exactly as before and no warning fires.
  • A descending (north-up) axis is regular even though its steps are negative, so the check compares step magnitudes against the averaged step. Without that, a normal north-up raster would warn falsely.

Why

Planar slope (and other distance-based tools that read calc_res) treated x=[0,1,2,4,8] with z=x as if every cell had the averaged width, so it returned a varying slope where the true physical slope is constant, with no signal to the user. attrs["res"] already gives an explicit override that get_dataarray_resolution honors before reaching calc_res, so the warning only fires when the grid is irregular and no res attr is set.

Backends

This is pure coordinate inspection in calc_res, so it's backend-agnostic. Coordinates are read as numpy regardless of array backend, so numpy / cupy / dask+numpy / dask+cupy all behave the same.

Test plan

  • Regular grid: no warning, resolution unchanged
  • Irregular x and irregular y: warns with the offending dim name
  • Descending (north-up) regular axis: no warning
  • Float regular grid (lat/lon): no warning
  • attrs["res"] set on an irregular grid: no warning
  • No spatial coords: no warning
  • End-to-end through slope on irregular coords: warns
  • Existing slope and rasterize/resample consumer suites pass

@github-actions github-actions Bot added the performance PR touches performance-sensitive code label Jun 1, 2026
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

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

PR Review: Warn when planar resolution averages irregular coordinates (#2766)

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

  • The coord.size < 3 guard in _warn_if_irregular_spacing (utils.py) means a 2-cell axis never warns. That's correct, since a 2-point axis has a single diff that always equals the averaged step, but a one-line comment saying so would save the next reader a moment of squinting.

Nits (optional improvements)

  • np.asarray(coord.values) would raise on a cupy-backed coordinate. In practice xrspatial coords stay numpy even for cupy/dask data, and the existing get_xy_range already calls .item() on them, so this matches the surrounding code and isn't a regression. Worth keeping in mind if coords ever move to the GPU.

What looks good

  • The descending-axis fix (comparing abs(diffs) to abs(res)) is the right call and has a regression test. Without it every north-up raster would warn falsely; the consumer suites confirm rasterize/resample no longer trip on it.
  • The warning only fires when there's no attrs['res'], because get_dataarray_resolution short-circuits on the attr before reaching calc_res. That path has a direct test.
  • Test coverage is thorough: regular, irregular x, irregular y, descending, float lat/lon, no-coords, res-attr override, and an end-to-end slope case.
  • The averaged resolution value is unchanged, so regular grids keep identical numeric behavior.

Checklist

  • Resolution value unchanged for regular grids
  • Descending/north-up axis handled (no false positive)
  • attrs['res'] override path tested
  • Edge cases covered (2-cell axis, no coords, float jitter)
  • Backend-agnostic (coordinate inspection only)
  • No new public API, so no README/docs change needed
  • Existing consumer suites (slope, rasterize, resample) pass

Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

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

Follow-up review (after commit 112aaa7)

Re-reviewed after the follow-up commit that comments the coord.size < 3 guard in _warn_if_irregular_spacing.

Blockers

None.

Suggestions

None left. The one accepted suggestion from the prior pass (comment the >= 3 point guard, utils.py:476-478) is in.

Nits

None left. The earlier np.asarray on cupy coords nit stays dismissed: xarray coordinate values are numpy-backed even for cupy/dask DataArrays, so np.asarray(coord.values) at utils.py:481 never touches device memory.

What looks good

  • Descending-axis handling compares abs(diffs) to abs(res), so north-up rasters don't warn falsely. Covered by test_calc_res_descending_axis_no_warning.
  • The rtol=1e-5 tolerance keeps float jitter from tripping the warning; test_calc_res_float_regular_grid_no_warning exercises it with realistic lat/lon spacing.
  • Missing coords, non-numeric dtype, non-finite diffs, and the <3 point case each return early.
  • The averaged resolution value is unchanged, so regular grids behave exactly as before. Only the warning is new.

Checklist

  • Algorithm matches intent (averaged-resolution detection)
  • Backend-agnostic (coords read as numpy regardless of array backend)
  • NaN/non-finite handling correct
  • Edge cases covered by tests (descending, float, no-coords, res-attr, end-to-end slope)
  • No premature materialization (pure coordinate inspection)
  • Docstrings present and accurate
  • No README/feature-matrix change needed (internal helper, no new public API)

Nothing actionable remains. Only the dismissed cupy nit stands.

@brendancol brendancol merged commit 85d91b3 into main Jun 1, 2026
7 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.

Planar slope silently averages resolution on irregular coordinates

1 participant