Warn on degree/meter unit mismatch in planar aspect()#2844
Merged
Conversation
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.
brendancol
commented
Jun 2, 2026
Contributor
Author
brendancol
left a comment
There was a problem hiding this comment.
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_resolutionin 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2839
What this does
aspect()'s planar path now callswarn_if_unit_mismatch(agg)before computing resolution, the same guardslope()already has. Degree-based lat/lon rasters with meter elevations get aUserWarninginstead of silently producing wrong values.aspect()docstring mirroring slope's, explaining the warning and pointing tomethod='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_elevationasserts the warning fires on a degree-coord meter-elevation rastertest_planar_no_warn_on_projected_meter_coordsasserts no warning on projected meter coordstest_aspect.pypasses (200 tests)Scope
Just the warning. The other aspect findings (dask memory guard, name leak, z_unit error message) are separate PRs.