Skip to content

Add degenerate-shape test coverage for D8 hydro functions (#2713)#2720

Open
brendancol wants to merge 3 commits into
xarray-contrib:mainfrom
brendancol:deep-sweep-test-coverage-hydro-d8-2026-05-29
Open

Add degenerate-shape test coverage for D8 hydro functions (#2713)#2720
brendancol wants to merge 3 commits into
xarray-contrib:mainfrom
brendancol:deep-sweep-test-coverage-hydro-d8-2026-05-29

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2713.

Adds tests for degenerate raster shapes (1x1, 1xN, Nx1) to four D8 hydrology functions that had no single-pixel or strip coverage: flow_accumulation_d8, flow_direction_d8, flow_length_d8, and hand_d8.

  • All four already handle these shapes without raising. The tests pin the current behavior so a regression in the kernel-boundary path gets caught.
  • flow_direction_d8: output is all-NaN at the input shape, since every cell is an edge cell.
  • flow_accumulation_d8, flow_length_d8, hand_d8: output is finite at the input shape. The flow_length test covers both direction modes.

Tests only, no source changes.

Backends: numpy. The degenerate-shape question is about kernel-boundary handling, and the numpy path is the reference. cupy, dask+numpy, and dask+cupy parity is already covered by existing tests in each of these files.

Test plan:

  • pytest on the four touched files passes on a CUDA host: 164 passed, including the 15 new cases. cupy and dask+cupy tests ran rather than skipped.

…trib#2713)

Adds 1x1, 1xN, and Nx1 regression tests for flow_accumulation_d8,
flow_direction_d8, flow_length_d8, and hand_d8. These functions already
handle single-pixel and strip inputs; the tests lock in that behavior so
a regression in the kernel-boundary path would be caught. Test-only, no
source changes.
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: Add degenerate-shape test coverage for D8 hydro functions (#2713)

Test-only change. Adds 1x1, 1xN, and Nx1 cases to four D8 test files. I read all four diffs against the source functions.

Blockers

None.

Suggestions

  • test_hand_d8.py test_degenerate_shape: this one only checks output shape. With fa=200 and threshold=100, every cell is a stream, so HAND should be 0 everywhere. Asserting that value would catch a regression that returns the wrong magnitude, not just the wrong shape. The other three tests check a value-level property (all-NaN, min >= 1, no NaN), so this is the odd one out.

Nits

  • All four tests use the same name (test_degenerate_shape) with near-identical bodies. Fine across separate files. If they ever get pulled into a shared parametrized helper, the per-function edge semantics (all-NaN vs finite) would need to stay distinct.

What looks good

  • Assertions match the current behavior for each function. I verified: flow_direction returns all-NaN because every cell is an edge cell, and the other three stay finite.
  • flow_length covers both direction modes.
  • numpy-only scope makes sense for a kernel-boundary check, and the PR body says so. Cross-backend parity is already covered by existing tests in each file.
  • Reuses the existing builders (_make_flow_dir_raster, _make_hand_rasters, create_test_raster), no new helpers.

Checklist

  • Assertions match verified current behavior
  • NaN handling correct (flow_direction all-NaN edge case)
  • Edge cases covered (this PR is the edge-case coverage)
  • [n/a] Dask chunk boundaries (numpy-only by design)
  • No premature materialization
  • [n/a] Benchmark (test-only)
  • [n/a] README matrix (no API change)
  • Docstrings present on each test

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 (#2713)

Addressed the one Suggestion from the first pass.

  • Fixed: test_hand_d8.py test_degenerate_shape now asserts HAND == 0 for the all-stream degenerate input, not just the output shape. Verified the value before adding the assert; the strengthened test passes.
  • Dismissed (with reason): the Nit about the shared test_degenerate_shape name. The four tests live in separate files and keep distinct per-function semantics (all-NaN for flow_direction, finite/zero for the others). No shared helper is planned, so the duplicate name is not a problem here.

No other findings remain.

@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 29, 2026
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.

Add degenerate-shape (1x1 / 1xN / Nx1) test coverage for D8 hydro functions

1 participant