Apply cell-size correction to planar aspect (#2760)#2780
Merged
Conversation
Planar aspect divided both Horn gradients by 8 without accounting for cellsize_x / cellsize_y, so it returned wrong directions for non-square cells. slope already did this correction; aspect did not. Thread cellsize_x / cellsize_y through the planar path the same way slope does: extract resolution in the public aspect(), pass it into the numpy/cupy/dask wrappers, and divide dz_dx by 8*cellsize_x and dz_dy by 8*cellsize_y in both the CPU and GPU kernels. Square cells are unchanged because the factor cancels in the atan2 ratio. northness() and eastness() are fixed for free since they derive from aspect(). Add a regression test: a constant-gradient raster with xres=10, yres=1 now yields aspect 315 (was 275.7106), with matching northness/eastness.
brendancol
commented
Jun 1, 2026
Contributor
Author
brendancol
left a comment
There was a problem hiding this comment.
PR Review: Apply cell-size correction to planar aspect (#2760)
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
None.
Nits (optional improvements)
- The aspect docstring still says the planar method "uses the classic Horn algorithm with uniform cell size". After this fix it handles non-uniform cells too. slope() carries the same stale wording, so leaving it keeps the two in sync; updating both would be a separate cleanup. Optional.
What looks good
- The fix mirrors the existing, tested slope() implementation: same 8cellsize_x / 8cellsize_y scaling, same get_dataarray_resolution call in the public function, same 1-element cupy arrays passed to the GPU kernel. Low risk because it copies a known-good pattern.
- The kernel orientation and compass conversion are untouched, so the QGIS reference tests still pass. Square cells are unchanged since the factor cancels in the atan2 ratio.
- All four backends are covered through the shared _cpu / _gpu kernels and the four wrapper functions; ArrayTypeFunctionMapping already dispatches to them.
- The regression test asserts the analytical Horn value (315.0) for a rectangular-cell raster rather than backend parity, and also checks northness/eastness. It was verified to fail on the unpatched kernel.
Notes checked, no action needed
- cellsize_y sign: calc_res derives yres from max-min of the y coords, so it is positive regardless of coordinate ordering, and the res attr is positive in practice. atan2 sign behavior matches slope's existing handling, so no new flip risk.
- Dask depth stays (1, 1) for the 3x3 kernel; boundary is forwarded unchanged. The cellsize values are bound via functools.partial, which is fine for map_overlap.
- Benchmarks already exist in benchmarks/benchmarks/aspect.py; no new function added, so no benchmark or README matrix change needed.
Checklist
- Algorithm matches the Horn cell-size correction used by slope()
- All implemented backends share the corrected kernels
- NaN handling unchanged (flat -> -1, edges -> NaN)
- Edge cases covered (existing degenerate-shape tests still pass)
- Dask chunk boundaries unchanged (depth 1, boundary forwarded)
- No premature materialization or extra copies
- Benchmark exists
- README feature matrix not applicable (no new function)
- Docstrings present (minor stale wording noted above)
brendancol
commented
Jun 1, 2026
Contributor
Author
brendancol
left a comment
There was a problem hiding this comment.
Follow-up review (after docstring commit)
The one nit from the first pass is fixed: the planar aspect docstring now says the gradients are scaled by the x and y cell sizes, so it no longer claims uniform cell size.
Nothing else changed in the code path. All 135 aspect tests still pass locally. No remaining findings.
# 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 #2760.
What
Planar
aspect()divided both Horn gradients by 8 without dividing bycellsize_x/cellsize_y, so it returned wrong directions for non-square cells.slope()already applied this correction; aspect did not.cellsize_x/cellsize_ythrough the planar aspect path the same wayslope()does: extract resolution in the publicaspect(), pass it into the numpy/cupy/dask backend wrappers, and dividedz_dxby8 * cellsize_xanddz_dyby8 * cellsize_yin both the CPU (_cpu) and GPU (_gpu/_run_gpu) kernels.northness()andeastness()are fixed for free since they derive fromaspect().Backend coverage
The fix lives in the shared
_cpuand_gpukernels, so all four backends benefit: numpy, cupy, dask+numpy, dask+cupy.Test plan