Skip to content

aspect: test degenerate shapes and geodesic boundary modes#2746

Open
brendancol wants to merge 3 commits into
mainfrom
deep-sweep-test-coverage-aspect-2026-05-29
Open

aspect: test degenerate shapes and geodesic boundary modes#2746
brendancol wants to merge 3 commits into
mainfrom
deep-sweep-test-coverage-aspect-2026-05-29

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2742

Adds the two missing test paths the deep-sweep test-coverage pass found in the aspect module. Test-only; no source changes.

  • Degenerate rasters (1x1, Nx1, 1xN) now have regression coverage for aspect, northness, and eastness. Below size 3 along a dimension the 3x3 kernel has no interior cell, so the correct output is all-NaN with the input shape preserved.
  • Geodesic boundary modes nearest, reflect, and wrap are now exercised. Before this, only boundary='nan' ran through the geodesic path, leaving its lat/lon padding code untested.

Backend coverage: both gaps are tested across numpy, cupy, dask+numpy, and dask+cupy. The cupy and dask+cupy cases were run and passed on a CUDA host (73 new cases, none skipped locally).

Test plan:

  • pytest xrspatial/tests/test_aspect.py xrspatial/tests/test_geodesic_aspect.py — 155 passed
  • new degenerate-shape cases pass on all four backends
  • new geodesic boundary cases pass on all four backends
  • cupy / dask+cupy cases observed running on GPU, not skipped

Two test-coverage gaps from the deep-sweep pass, test-only:

- Degenerate rasters (1x1, Nx1, 1xN) had no coverage. A 3x3 kernel has
  no interior cell below size 3, so the correct result is all-NaN with
  the input shape preserved. Added regression tests for aspect,
  northness, and eastness across numpy, cupy, dask+numpy, dask+cupy.
- Geodesic boundary modes (nearest, reflect, wrap) were untested; only
  'nan' ran. Added a numpy mode test plus numpy-vs-dask, cupy, and
  dask+cupy parity for the non-default modes.

GPU-validated: all new cupy and dask+cupy cases ran and passed on a
CUDA host.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 30, 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: aspect test coverage (degenerate shapes and geodesic boundary modes)

Test-only PR adding two missing coverage paths to the aspect module. No source changes.

Blockers

None.

Suggestions

One note for the record: in the degenerate-shape tests, the data[0, :] = 2.0 tweak only runs when shape[0] > 1, so the (1, 5) and (1, 3) cases feed a flat all-ones raster. The all-NaN assertion still holds there, but for the no-interior-cell reason rather than a flat-surface reason. That is the intended geometric edge case, so it is fine as written.

Nits

  • The new _to_numpy helper in test_aspect.py duplicates the backend-unwrap logic that general_checks.py already does inline. Not worth extracting for three call sites, but it could fold into a shared helper later.

What looks good

  • Both gaps are covered across all four backends (numpy, cupy, dask+numpy, dask+cupy) using the existing create_test_raster helper and the standard skip decorators.
  • The cupy and dask+cupy cases ran on a CUDA host and passed, not just added behind a skip.
  • Geodesic boundary tests assert the actual contract: non-default modes fill the edge ring, nan keeps it NaN, plus numpy-vs-dask/cupy parity.
  • Dask chunks are clamped to the raster size for the degenerate shapes, so no chunk-larger-than-array warnings.

Checklist

  • Algorithm matches reference: n/a (test-only)
  • All implemented backends consistent: yes, parity asserted
  • NaN handling correct: yes, that is what the tests check
  • Edge cases covered: yes, that is the point of this PR
  • Dask chunk boundaries handled: yes, chunks clamped
  • No premature materialization: compute only at assertion time
  • Benchmark: not needed (test-only)
  • README matrix: not needed (no new function)
  • Docstrings: n/a

…age-aspect-2026-05-29

# Conflicts:
#	xrspatial/tests/test_aspect.py
- numpy degenerate-shape test now runs general_output_checks to assert
  attrs/coords/dims are preserved, matching the slope #2703 convention.
- add a geodesic degenerate-shape test (1x1, 1xN, Nx1) so the no-interior
  case is pinned for method='geodesic' too, not just planar.
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 (post-merge pass): aspect test coverage

Re-reviewed after merging origin/main, which brought in #2741 (planar dask now reports float32) and #2703 (slope's degenerate-shape convention). Resolved the test_aspect.py conflict by keeping both #2741's dtype tests and the degenerate-shape tests. Still test-only.

Blockers

None.

Suggestions (addressed in 8dbf656)

  • The numpy degenerate-shape test asserted only shape and all-NaN. #2703's slope version also runs general_output_checks to catch a metadata-dropping regression. Added that call. Verified it passes for aspect, northness, and eastness on every degenerate shape.
  • The geodesic path had boundary-mode coverage but no degenerate-shape test, while slope (#2703) added one for its geodesic method. Added a (1x1, 1xN, Nx1) geodesic test so the no-interior case is pinned there too.

Nits (declined)

  • The _to_numpy helper in test_aspect.py overlaps with the assert_numpy_equals_* parity helpers. Kept it because the degenerate tests are parametrized over three functions and five shapes, and the small unwrap helper reads more directly than routing every case through the parity asserts. Not worth the churn.

What looks good

  • All four backends covered for both gaps; cupy and dask+cupy ran on a CUDA host (60 degenerate cases plus the geodesic boundary set, none skipped locally).
  • Dask chunks clamped to raster size on the degenerate shapes, so no chunk-larger-than-array warnings.
  • The merge kept #2741's float32 dtype assertions intact and they still pass.

Checklist

  • Algorithm matches reference: n/a (test-only)
  • Backends consistent: yes, parity asserted
  • NaN handling: yes, that is what the tests check
  • Edge cases covered: yes, including geodesic degenerate shapes now
  • Dask chunk boundaries: yes, clamped
  • No premature materialization: compute only at assertion time
  • Benchmark: not needed (test-only)
  • README matrix: not needed
  • Docstrings: n/a

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.

aspect: add test coverage for degenerate raster shapes and geodesic boundary modes

1 participant