Add pathfinding test coverage: dask edge handling, degenerate shapes, parameter branches#3665
Open
brendancol wants to merge 2 commits into
Open
Add pathfinding test coverage: dask edge handling, degenerate shapes, parameter branches#3665brendancol wants to merge 2 commits into
brendancol wants to merge 2 commits into
Conversation
…parameter branches (#3657) Branch coverage 86% -> 96%. New tests: dask barrier/NaN/no-path/warning paths, 1x1 and strip rasters on all backends, friction and search_radius parameter branches, cupy snap/auto-radius (run on GPU), HPA* with friction. Test-only change. Claude-Session: https://claude.ai/code/session_0155N4QGamQVxgpAAPbpQNq4
brendancol
commented
Jul 8, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review: Add pathfinding test coverage: dask edge handling, degenerate shapes, parameter branches
Test-only PR (plus the sweep state CSV). No source changes, so the review focuses on whether the new tests are correct, deterministic, and actually exercise the paths they claim to.
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
- The new dask edge tests (barriers, NaN wall, non-crossable warnings, blocked start) all run on dask+numpy only. The dask+cupy path shares
_a_star_daskbut takes theis_cupy=Truebranch in_load_chunk(block.get()) and builds output blocks through_make_block_cupy. One barrier-grid parity test on dask+cupy would close that last gap cheaply (xrspatial/tests/test_pathfinding.py:1210, pattern already exists intest_dask_cupy_matches_numpy).
Nits (optional improvements)
-
test_degenerate_shapes_cupycovers 1x1 and 1x5 but not 5x1 (xrspatial/tests/test_pathfinding.py:1330). The cupy backend transfers to the same numpy kernel, so the risk is low; adding the column case would make the three shape tests symmetric across backends. -
test_hpa_star_with_frictionasserts the goal is reached but not that friction influenced the route (xrspatial/tests/test_pathfinding.py:1520). Hard to assert tightly since HPA* is heuristic, and the exact-value_coarsen_frictioncheck compensates. Fine as is; noting it so nobody mistakes it for a routing-quality test.
What looks good
- Every test in the diff was verified by execution: 87 passed with JIT enabled, and the coverage claim (86% -> 96% branch) reproduces with
NUMBA_DISABLE_JIT=1. - The dask tests target the right thing:
_a_star_daskis a separate pure-Python implementation, and the new barrier-parity test compares it against the numpy kernel on the same grid rather than just checking it runs. - Expected values are concrete where they can be: unit-step costs on strips, exact
_coarsen_frictionblock mean of 2.0, all-NaN no-path contracts. - The memory-mock tests patch
_available_memory_byteswith values chosen so the auto-radius arithmetic is deterministic (checked: 20x20x65 = 26000 against the 20000/32000 thresholds). - Deliberate exclusions are documented: the dims-mismatch message test is deferred because #3649 rewords it, which avoids a cross-PR conflict.
- Pre-existing flake8 findings in the file were left untouched; the new code adds none.
Checklist
- Algorithm matches reference/paper (n/a, test-only)
- All implemented backends produce consistent results (numpy/dask/cupy/dask+cupy parity tests run locally)
- NaN handling is correct (NaN wall, NaN friction, snap-over-NaN all covered)
- Edge cases are covered by tests (1x1, 1xN, Nx1 added)
- Dask chunk boundaries handled correctly (barrier grid tested at (2,2) chunks)
- No premature materialization or unnecessary copies (n/a, test-only)
- Benchmark exists or is not needed (benchmarks tracked separately in #3645 / PR #3648)
- README feature matrix updated (n/a, no API change)
- Docstrings present and accurate (each test has a one-line docstring)
Contributor
Author
|
Follow-up 155a1da addresses the review: added test_dask_cupy_barriers_match_numpy (barrier parity through the cupy chunk-conversion branch of _a_star_dask) and the 5x1 column case in test_degenerate_shapes_cupy. 88 tests pass locally, GPU tests included. The HPA* nit is left as documented. |
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 #3657
Test-only change; no source edits. Adds 30 tests to
test_pathfinding.py, taking measured branch coverage ofxrspatial/pathfinding.pyfrom 86% to 96% (57 -> 87 tests, all passing)._a_star_dask), and none of its barrier, NaN-wall, no-path, non-crossable-start/goal, or friction-blocked-start branches had a test. New tests assert parity with numpy on a barrier grid plus each of the empty/warning outcomes.friction has no positive finite valueson numpy/dask/cupy,search_radiuscombined with friction,multi_stop_search(snap=True)goal remap, optimize_order warning, waypoint length check, snap+dask raise, numpy friction with dask surface._hpa_star_searchwith a friction surface, plus a direct_coarsen_frictioncheck (that kernel had zero coverage).Backend coverage: numpy, dask+numpy, cupy, and dask+cupy paths all exercised; the cupy tests ran for real on a CUDA box.
Deliberately not tested here: the dims-mismatch error message (being reworded in #3649) and the LOW-severity branches listed in the issue (chunk-cache eviction, TSP shortcut branches, non-square cellsizes).
Test plan:
pytest xrspatial/tests/test_pathfinding.py— 87 passed, GPU tests includedNUMBA_DISABLE_JIT=1 pytest --cov=xrspatial.pathfinding --cov-branch— 96%https://claude.ai/code/session_0155N4QGamQVxgpAAPbpQNq4