Skip to content

Clear the pathfinding LOW backlog: perf nits, empty-raster error, test and bench gaps - #3706

Merged
brendancol merged 2 commits into
mainfrom
issue-3705
Aug 16, 2026
Merged

Clear the pathfinding LOW backlog: perf nits, empty-raster error, test and bench gaps#3706
brendancol merged 2 commits into
mainfrom
issue-3705

Conversation

@brendancol

@brendancol brendancol commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Closes #3705.

Works through the bundled LOW backlog from the 2026-07-08 sweep round in one pass.

Performance

  • _nearest_neighbor_2opt now 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 < inf is 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_dask fetches the popped node's friction value once per node instead of once per neighbor.
  • The dask start/goal crossability checks run as a single dask.compute instead of two scheduler round-trips.
  • Dask friction f_min is cached in a small LRU keyed by the dask array's graph token, so an N-waypoint multi_stop_search scans 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.
  • The numba kernel's parent index arrays use np.full instead of np.ones * NONE.

Error message

  • A zero-size surface now raises ValueError: ... surface is empty (shape (0, 0)) from _validate_surface_dims in both public functions, instead of leaking numpy.nanmin raises on a.size==0 from the resolution machinery.

Tests (11 new, file went from 141 to 152 passing locally on a CUDA host)

  • _ChunkCache LRU eviction and refresh-on-hit
  • _held_karp n==2 shortcut, both directions
  • 2-opt improvement branch: a hand-built matrix where greedy nearest-neighbor is wrong and one reversal fixes it; seeded asymmetric matrices asserting the returned cost matches the recomputed tour cost and never exceeds the greedy tour; an inf-edge case pinning the nan-poisoning guard
  • _get_pixel_id default dim arguments
  • empty surfaces for a_star_search and multi_stop_search
  • f_min cache behavior plus a dask-vs-numpy multi_stop_search friction parity test that asserts all three segments resolve through one cache entry

Review follow-up: the posted review's suggestion (assert cache reuse across segments) and docstring nit are applied; the pre-existing common.py guard bug is filed as #3707.

Benchmarks

  • New 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_dataarray gains a dask+cupy type (verified cupy-backed via is_dask_cupy), wired into AStarSearch. Other benchmark classes can opt in later.

Not fixed here, spotted next to the change: common.py's cupy branch tests if not has_cuda_and_cupy: without calling the function, so it never raises NotImplementedError on 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)
  • New benchmark classes instantiated and run once per backend, including dask+cupy
  • flake8/isort clean on the added code (pre-existing findings in the test file untouched)

…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 brendancol left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_cache before the dask run and asserting len(_dask_fmin_cache) == 1 afterward 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 because inf < inf never 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 tests if not has_cuda_and_cupy: without calling the function, so it can never raise NotImplementedError and a non-GPU host gets an ImportError instead. The PR body flags it and correctly calls the function in the new dask+cupy branch; 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 = nan would 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_val hoist in _a_star_dask is 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) and rechunk (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.
  • AStarSearchObstacles fixes 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+cupy benchmark type was verified genuinely cupy-backed (is_dask_cupy true, cupy _meta), not silently coerced to numpy blocks by from_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.

@brendancol brendancol left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_numpy now clears _dask_fmin_cache before 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.py guard 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.

@brendancol
brendancol merged commit 59b16bc into main Aug 16, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pathfinding: LOW-severity cleanup backlog from the 2026-07-08 sweep round

1 participant