Add golden-value reference tests for a_star_search (#3655)#3663
Merged
brendancol merged 3 commits intoJul 9, 2026
Merged
Conversation
Pin goal costs on small inputs (open grid, barrier maze, friction surface, anisotropic cellsize, 4/8-connectivity, no-path case) to values verified against scipy 1.16.1 sparse.csgraph.dijkstra and scikit-image 0.26.0 MCP_Geometric, which use the same edge model. Test-only; no source changes. 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 golden-value reference tests for a_star_search (#3655)
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
-
xrspatial/tests/test_pathfinding.py:1193:_golden_rasteroverlaps with the existing_make_rasterhelper a few hundred lines up. The overlap is deliberate and correct:_make_rasterwritesattrs={'res': res}withres[0]used as the y spacing, whileget_dataarray_resolution(utils.py:685) readsattrs['res']as(cellsize_x, cellsize_y). With square cells that mismatch is invisible; with the anisotropic case in this PR it would flip cx and cy._golden_rasteravoids the trap by setting noresattr, so resolution comes from the coords. A one-line comment in_golden_rastersaying why it does not reuse_make_rasterwould keep someone from "simplifying" this later and silently breaking the anisotropic test.
Nits (optional improvements)
-
xrspatial/tests/test_pathfinding.py:1203-1226: thecasetuple element exists only to name the parametrization.pytest.param(..., id=...)expresses that directly and drops the unused function argument. Purely cosmetic; the current form is readable as-is.
What looks good
- Golden values are traceable: the block comment names both reference tools with exact versions (scipy 1.16.1 dijkstra, scikit-image 0.26.0 MCP_Geometric), the generation date, and the observed agreement (2e-15 rel or better), and the issue carries the full runnable comparison script.
- Goal-cost pinning is the right invariant: optimal cost is unique even when the optimal path is not, so the tests will not flake on heap tie-breaking changes.
- Maze and friction grids are written out as literals rather than regenerated from
RandomState, so the tests do not depend on numpy RNG stream stability. - Coverage picks the spots where a cost-model regression would hide: 4- vs 8-connectivity, barriers, friction weighting, anisotropic cellsize, and a no-path case asserting the all-NaN contract.
- Runs on numpy and dask+numpy; the existing cupy/dask+cupy parity tests extend the pinned values to GPU backends without duplicating them here.
- Test-only diff plus the sweep state CSV row; no source or dependency changes.
Checklist
- Algorithm matches reference/paper (verified against two independent implementations plus closed form)
- All implemented backends produce consistent results (numpy/dask direct; cupy via existing parity tests)
- NaN handling is correct (no-path case asserts all-NaN output)
- Edge cases are covered by tests (no-path, anisotropic cells, 4-connectivity)
- Dask chunk boundaries handled correctly (dask param uses non-trivial chunks)
- No premature materialization or unnecessary copies (test code;
.valueson small grids is fine) - Benchmark exists or is not needed (test-only change; #3645 tracks pathfinding benchmarks separately)
- README feature matrix updated (not applicable, no new function)
- Docstrings present and accurate (not applicable: no public API change)
31fad83 to
7847ffe
Compare
brendancol
commented
Jul 8, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review: follow-up after review commits
Both findings from the first review are addressed in 7847ffe:
- Suggestion (fixed):
_golden_rasternow carries a comment explaining why it does not reuse_make_raster(theattrs['res']ordering mismatch withget_dataarray_resolutionthat would flip cx/cy on anisotropic cells). - Nit (fixed): the golden parametrizations use
pytest.param(..., id=...); the unusedcaseargument is gone.
One process note: an intermediate commit accidentally rewrote the test file's CRLF line endings to LF, which ballooned the diff to ~1350 changed lines. That was caught and amended; the follow-up commit is back to a 23+/18- diff with the file's original CRLF endings intact.
Re-verified after the changes: 75 passed in xrspatial/tests/test_pathfinding.py, flake8 finding count unchanged (14 pre-existing, none in the added block).
No remaining findings.
3 tasks
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 #3655
a_star_search: goal costs on a 6x6 open grid (8- and 4-connectivity, matches the closed form), an 8x8 barrier maze (including a no-path pair), a 6x6 friction surface, and an anisotropic-cellsize grid.sparse.csgraph.dijkstraand scikit-image 0.26.0MCP_Geometric(same edge model: geometric distance, or distance times mean endpoint friction). The values are hardcoded, so neither reference library is needed at test time.Backend coverage: the new tests run on numpy and dask+numpy. The existing
test_cupy_matches_numpy/test_dask_cupy_matches_numpyparity tests carry the pinned values to the GPU backends.Also updates the reference-validation sweep state CSV row for
pathfinding(verdict MATCHES).Test plan:
pytest xrspatial/tests/test_pathfinding.py -k golden(18 passed)pytest xrspatial/tests/test_pathfinding.py(75 passed, includes cupy and dask+cupy on a CUDA host)https://claude.ai/code/session_0155N4QGamQVxgpAAPbpQNq4