Skip to content

aspect: add independent rectangular-cell oracle for planar aspect#2781

Open
brendancol wants to merge 3 commits into
mainfrom
issue-2768
Open

aspect: add independent rectangular-cell oracle for planar aspect#2781
brendancol wants to merge 3 commits into
mainfrom
issue-2768

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2768

What this does

The planar aspect tests only prove the four backends agree with each other. They share the same _cpu/_gpu kernels, so a defect common to every backend slips through. The missing-cellsize bug (#2760) is exactly that: planar aspect divides the Horn gradients by 8 but never by cellsize_x/cellsize_y, so it is wrong on non-square cells while every backend still matches. The QGIS fixture is a real external reference but uses square cells, so it never touches rectangular cells or checks that cell size comes from the coordinates.

This adds an analytic oracle for planar aspect:

  • Builds a plane z = gx*X + gy*Y on a coordinate grid with no res attr, so resolution must be recovered from the x/y coordinate spacing.
  • Asserts against the closed-form Horn compass value, not against another backend.
  • Square-cell oracle passes unconditionally.
  • Rectangular-cell oracle is xfail(strict=False) referencing Planar aspect() ignores cell size; wrong for non-square cells #2760. It flips to XPASS automatically once that fix lands, which flags the marker for removal.

This PR is test-only. It does not touch aspect.py; the source fix is #2760's scope.

Backend coverage

numpy, dask+numpy, cupy, dask+cupy. The cupy/dask+cupy oracle variants are gated behind the existing CUDA+CuPy skip marker.

Test plan

  • pytest xrspatial/tests/test_aspect.py -- 165 passed, 16 xfailed (numpy + dask; cupy paths skipped, no GPU here)
  • Square-cell oracle passes on numpy and dask
  • Rectangular-cell oracle xfails against current main as expected (confirmed it reproduces the Planar aspect() ignores cell size; wrong for non-square cells #2760 buggy value 275.71 vs oracle 315.0 for xres=10, yres=1)
  • flake8 clean on the added lines

When #2760 merges, the rectangular oracle should start XPASSing; remove the xfail markers at that point.

)

The planar tests only check that the four backends agree. They share the
same _cpu/_gpu kernels, so a shared defect like the missing-cellsize bug
(#2760) passes anyway. The QGIS fixture is a real reference but uses
square cells, so it never exercises rectangular cells or checks that cell
size is read from the coordinates.

Add an analytic oracle: build a plane z = gx*X + gy*Y on a coordinate
grid with no 'res' attr (so resolution comes from the x/y spacing) and
compare aspect() against the closed-form Horn compass value instead of
against another backend.

- Square-cell oracle passes unconditionally on numpy, dask, cupy, dask+cupy.
- Rectangular-cell oracle is xfail(strict=False) pending #2760; it flips
  to XPASS automatically once that fix lands, flagging the marker for
  removal.
@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: aspect: add independent rectangular-cell oracle for planar aspect

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

  • _aspect_oracle hand-copies the compass conversion from aspect._cpu (lines 74-81). That duplication is the whole point of an independent oracle, but it means the two can drift apart if the source convention ever changes. The square-cell tests re-check the oracle against the real function on every run, so this is fine as written. A one-line comment saying the conversion is duplicated on purpose would save the next reader some confusion. (xrspatial/tests/test_aspect.py:265)
  • The flat case gx == gy == 0 has a branch in _aspect_oracle but no test feeds it, so the -1 path is dead code in this PR. Either drop the branch or add a (0.0, 0.0) parametrize case. A flat-surface oracle case is cheap and worth having. (xrspatial/tests/test_aspect.py:271)

Nits (optional improvements)

  • In the cupy oracle tests, the inner if backend == 'dask+cupy' and not has_dask_array(): pytest.skip(...) only matters on a machine with CuPy but no dask, which is rare. Harmless, just slightly defensive. (xrspatial/tests/test_aspect.py:325)

What looks good

  • The oracle asserts against an analytic value, not another backend. That is the gap the issue called out.
  • Rectangular cells with no res attr force resolution to come from coordinate spacing, which is the exact path the missing-cellsize bug breaks.
  • The xfail(strict=False) pointing at #2760 is the right call: green CI now, automatic XPASS when the fix lands.
  • Reuses _to_numpy and the existing skip markers; coverage spans numpy, dask, cupy, dask+cupy.

Checklist

  • Oracle matches the Horn compass convention in aspect._cpu
  • Square-cell oracle passes on all available backends
  • Rectangular-cell oracle xfails pending #2760
  • Resolution is taken from coordinates (no res attr)
  • Test-only change; aspect.py untouched
  • flake8 clean on added lines
  • Flat-surface (-1) oracle case not exercised

…2768)

- Add (0.0, 0.0) to the square oracle params so the flat-surface -1 branch
  in _aspect_oracle is actually tested (it was dead code before).
- Note in the docstring that the arctan2/compass conversion is duplicated
  from aspect._cpu on purpose, with the square tests guarding against drift.
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

The two suggestions from the first pass are addressed in 12be968:

  • The flat-surface case (0.0, 0.0) is now in the square oracle params, so the -1 branch in _aspect_oracle is exercised. xrspatial returns -1 for a constant plane and the oracle agrees.
  • The docstring now says the arctan2/compass conversion is duplicated from aspect._cpu on purpose, with the square tests guarding against drift.

The one remaining nit (the defensive has_dask_array() skip inside the cupy tests) is left as-is on purpose. It only matters on a CuPy-without-dask machine, and keeping it costs nothing while avoiding a confusing failure in that rare setup.

Oracle tests: 42 passed, 16 xfailed (numpy + dask; cupy paths skipped, no GPU here). flake8 clean on the added lines. No new findings.

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 tests: add an independent rectangular-cell oracle (parity-only coverage gap)

1 participant