Skip to content

Warn on degree/meter unit mismatch in planar aspect()#2844

Merged
brendancol merged 2 commits into
mainfrom
issue-2839
Jun 3, 2026
Merged

Warn on degree/meter unit mismatch in planar aspect()#2844
brendancol merged 2 commits into
mainfrom
issue-2839

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2839

What this does

  • aspect()'s planar path now calls warn_if_unit_mismatch(agg) before computing resolution, the same guard slope() already has. Degree-based lat/lon rasters with meter elevations get a UserWarning instead of silently producing wrong values.
  • Adds a "Notes" paragraph to the aspect() docstring mirroring slope's, explaining the warning and pointing to method='geodesic' or reprojection.

Backend coverage

The warning lives in the shared aspect() entry point ahead of backend dispatch, so it fires for all four backends (numpy / cupy / dask+numpy / dask+cupy). No backend kernels changed.

Test plan

  • test_planar_warns_on_degree_coords_meter_elevation asserts the warning fires on a degree-coord meter-elevation raster
  • test_planar_no_warn_on_projected_meter_coords asserts no warning on projected meter coords
  • Full test_aspect.py passes (200 tests)

Scope

Just the warning. The other aspect findings (dask memory guard, name leak, z_unit error message) are separate PRs.

aspect()'s planar path went straight to get_dataarray_resolution without
checking whether horizontal units (degrees) match vertical units (meters).
slope() already calls warn_if_unit_mismatch() before computing resolution.
Reuse that same helper in aspect so degree/meter rasters get a UserWarning.

Adds tests asserting the warning fires on a degree-coord meter-elevation
raster and stays silent on projected meter coords, mirroring the slope
tests.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label Jun 2, 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 on degree/meter unit mismatch in planar aspect()

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

One observation, not blocking: the new test helpers _degree_coord_meter_elevation_raster and _projected_meter_raster in test_aspect.py are copies of the same helpers in test_slope.py. The duplication keeps the aspect test file self-contained, which is how the suite is already organized, so it's fine as-is.

Nits (optional improvements)

  • The shared warning message in utils.py ends with "before calling slope" even when the call comes from aspect. That wording predates this PR and lives in the shared helper, so leaving it is consistent. A future cleanup could make it backend-agnostic.
  • A few existing oracle tests (the cellsize=0.5 variants) now surface this UserWarning because their coordinate spacing looks degree-like to the heuristic. The tests still pass; that's the heuristic doing its job, not a regression.

What looks good

  • The fix mirrors slope.py: same helper, same import source, same placement just before get_dataarray_resolution in the planar branch.
  • The warning sits in the shared aspect() entry point ahead of backend dispatch, so all four backends (numpy, cupy, dask+numpy, dask+cupy) get it with no kernel changes.
  • Tests cover both directions: the warning fires on degree/meter coords and stays silent on projected meter coords. Both pass, and the full aspect suite (200 tests) is green.
  • Added a Notes paragraph to the aspect docstring matching slope's.

Checklist

  • No algorithm change; warning only
  • All backends consistent (warning is pre-dispatch)
  • NaN handling unchanged
  • Edge cases covered (warn / no-warn both tested)
  • Dask chunk boundaries unchanged
  • No premature materialization
  • Benchmark not needed
  • README feature matrix not applicable (no new function)
  • Docstrings present and accurate

# Conflicts:
#	xrspatial/aspect.py
#	xrspatial/tests/test_aspect.py
@brendancol brendancol merged commit 5dfbf8c into main Jun 3, 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 aspect() does not warn on degree/meter unit mismatch like slope() does

1 participant