Clear the pathfinding LOW backlog: perf nits, empty-raster error, test and bench gaps - #3706
Conversation
…t and bench gaps (#3705) - Score 2-opt candidate moves in O(1) via inf-aware prefix sums instead of re-summing the whole tour twice per candidate - Hoist the popped node's friction fetch out of the dask A* neighbor loop - Build parent index arrays with np.full instead of np.ones * NONE - Batch the dask start/goal crossability reads into one compute - Cache dask friction f_min by graph token so multi-stop routes scan the friction raster once instead of once per segment - Raise a clear ValueError for zero-size surfaces instead of leaking a numpy nanmin error - Tests: _ChunkCache eviction, _held_karp n==2, the 2-opt improvement branch (incl. asymmetric and inf-edge cases), _get_pixel_id default dims, empty surfaces, dask friction f_min cache - Benchmarks: barrier+friction A* variant (open grid is A*'s best case) and a dask+cupy array type wired into AStarSearch
brendancol
left a comment
There was a problem hiding this comment.
PR Review: Clear the pathfinding LOW backlog
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
-
xrspatial/tests/test_pathfinding.py(test_multi_stop_dask_friction_matches_numpy): the test proves dask/numpy parity but never asserts the f_min cache was actually hit across segments, which is the point of routing 3 segments through one friction raster. Clearing_dask_fmin_cachebefore the dask run and assertinglen(_dask_fmin_cache) == 1afterward would pin the cross-segment reuse; today only the graph-token unit test covers it, and a future refactor that silently changes the token per call (e.g. rechunk to different chunks) would regress the optimization with no test failing.
Nits (optional improvements)
-
xrspatial/pathfinding.py:1444(_nearest_neighbor_2opt): the behavior change for inf-edge tours (previously frozen becauseinf < infnever accepted a move; now finite local improvements still apply) lives in a code comment and the PR body. One sentence in the function docstring would make it visible to the next reader without archaeology. -
benchmarks/benchmarks/common.py:49: the pre-existing cupy branch testsif not has_cuda_and_cupy:without calling the function, so it can never raiseNotImplementedErrorand a non-GPU host gets an ImportError instead. The PR body flags it and correctly calls the function in the newdask+cupybranch; fine to leave for a separate fix, noting it here for the trail.
What looks good
- The 2-opt rewrite handles the case a naive prefix-sum delta gets wrong: with unreachable waypoint pairs,
inf - inf = nanwould silently reject valid moves. The inf-count-plus-finite-sum bookkeeping keeps range sums exact, and the inf-edge test pins exactly that failure mode. Termination still holds since each accepted move strictly decreases (inf-edge count, finite cost) lexicographically. - The asymmetric consistency test asserts the returned cost equals the cost recomputed from the returned order, which is the invariant a delta-bookkeeping bug would break, and it replicates the greedy construction to assert 2-opt never worsens it.
- The
f_u_valhoist in_a_star_daskis safe: the goal check returns before the fetch, and any node in the heap already passed the positive-finite friction check when it was enqueued as a neighbor (or is the start, checked before the loop). - The f_min cache key is the dask graph token, which is content-derived for both
da.from_array(numpy friction) andrechunk(dask friction), so a rebuilt or different raster gets a fresh entry rather than a stale hit. Capped at 16 entries with LRU eviction. - The empty-surface check sits in
_validate_surface_dims, which both public functions call before any resolution math, so(0, 0),(0, N), and(N, 0)all get the clear message; all three shapes are parametrized in the test. AStarSearchObstaclesfixes a real blind spot: the open-grid benchmark lets the heuristic walk nearly straight to the goal, so frontier-growth regressions were invisible. The serpentine wall layout forces exploration, and the friction variant exercises the weighted-cost path.- The new
dask+cupybenchmark type was verified genuinely cupy-backed (is_dask_cupytrue, cupy_meta), not silently coerced to numpy blocks byfrom_array.
Checklist
- Algorithm matches reference: 2-opt/Held-Karp semantics preserved; delta scoring verified equal to full recomputation by test
- All implemented backends produce consistent results: 152 existing + 10 new tests pass on a CUDA host (cupy and dask+cupy executed)
- NaN handling is correct: no changes to NaN semantics; inf handling in the TSP path improved and tested
- Edge cases are covered by tests: empty surfaces, two-city TSP, inf edges, cache eviction, default dims
- Dask chunk boundaries handled correctly: no chunking changes; crossability reads batched into one compute
- No premature materialization: the batched compute and f_min cache reduce materialization; nothing new added
- Benchmark exists: two new benchmark surfaces added
- README feature matrix: not applicable, no new public functions or backend-support changes
- Docstrings: no public signatures changed; new private helpers have docstrings
No blockers. The one suggestion is a cheap test hardening; the nits are optional.
…f-edge 2-opt behavior (#3705)
brendancol
left a comment
There was a problem hiding this comment.
PR Review: follow-up on 73b9a18
Re-review of the changes since the first review.
Blockers (must fix before merge)
None.
Suggestions / Nits
None new.
Disposition of the first review's findings
- Suggestion (cache-reuse assertion): fixed.
test_multi_stop_dask_friction_matches_numpynow clears_dask_fmin_cachebefore the dask run and asserts exactly one entry after routing three segments, which pins the cross-segment reuse the optimization exists for. A future change that varies the graph token per call now fails this test. - Nit (inf-edge docstring): fixed.
_nearest_neighbor_2opt's docstring states that inf-edge tours still receive finite local improvements and keep an inf total. - Nit (pre-existing
common.pyguard bug): deferred to #3707, as the first review itself recommended.
One note on the commit's diff shape
The commit shows ~195 removed/re-added lines in test_pathfinding.py that are pure line-ending normalization: the file is CRLF on main, the first commit appended the new tests with LF endings, and this commit's edit rewrote them as CRLF. The net PR diff against main is 202 pure additions with zero deletions and consistent CRLF, so nothing to fix; noting it so the per-commit history doesn't read as a rewrite.
Full pathfinding suite re-run after the changes: 152 passed on a CUDA host. No blockers; the PR is in good shape from this reviewer's side.
Closes #3705.
Works through the bundled LOW backlog from the 2026-07-08 sweep round in one pass.
Performance
_nearest_neighbor_2optnow scores each 2-opt candidate in O(1) from prefix sums over the current tour's forward and backward edge costs, instead of re-summing the whole tour twice per candidate. Infinite edges (unreachable waypoint pairs) are tracked as counts next to the finite sums, since subtracting two prefix sums that both contain inf gives nan and would silently reject valid moves. One behavior change in the degenerate case: a tour stuck with an unavoidable inf edge previously could never improve (inf < infis always false); it now still applies finite local improvements. Termination holds because each accepted move strictly decreases (inf-edge count, finite cost) lexicographically._a_star_daskfetches the popped node's friction value once per node instead of once per neighbor.dask.computeinstead of two scheduler round-trips.f_minis cached in a small LRU keyed by the dask array's graph token, so an N-waypointmulti_stop_searchscans the friction raster once instead of N-1 times (the follow-up noted in multi_stop_search materializes the full grid on dask backends, defeating the sparse A* design #3660). The token is content-derived, so a different or rebuilt raster gets a fresh entry.np.fullinstead ofnp.ones * NONE.Error message
ValueError: ... surface is empty (shape (0, 0))from_validate_surface_dimsin both public functions, instead of leakingnumpy.nanmin raises on a.size==0from the resolution machinery.Tests (11 new, file went from 141 to 152 passing locally on a CUDA host)
_ChunkCacheLRU eviction and refresh-on-hit_held_karpn==2 shortcut, both directions_get_pixel_iddefault dim argumentsa_star_searchandmulti_stop_searchf_mincache behavior plus a dask-vs-numpymulti_stop_searchfriction parity test that asserts all three segments resolve through one cache entryReview follow-up: the posted review's suggestion (assert cache reuse across segments) and docstring nit are applied; the pre-existing
common.pyguard bug is filed as #3707.Benchmarks
AStarSearchObstacles: serpentine barrier walls plus a friction gradient, since the existing open-grid benchmark is A*'s best case and hides regressions in barrier handling and frontier growth.common.get_xr_dataarraygains adask+cupytype (verified cupy-backed viais_dask_cupy), wired intoAStarSearch. Other benchmark classes can opt in later.Not fixed here, spotted next to the change:
common.py's cupy branch testsif not has_cuda_and_cupy:without calling the function, so it never raisesNotImplementedErroron non-GPU hosts and falls through to an ImportError instead. Pre-existing, left alone to keep the diff scoped.Backend coverage: no dispatch changes; numpy/cupy/dask+numpy/dask+cupy all exercised by the existing suite plus the new tests, run locally on a CUDA host.
Test plan:
pytest xrspatial/tests/test_pathfinding.py— 152 passed (CUDA host, GPU tests executed)dask+cupy