Pathfinding API consistency: preserve attrs in multi_stop_search, Dataset support for a_star_search, type hints, defaults#3653
Merged
brendancol merged 2 commits intoJul 9, 2026
Conversation
…ints, defaults (#3644) - multi_stop_search now preserves input surface attrs alongside waypoint_order/segment_costs/total_cost - a_star_search gets @supports_dataset like its sibling and the rest of the library; ds.xrs.a_star_search added to the Dataset accessor - start/goal annotated np.ndarray (was np.array, a function), x/y annotated plain str (None never worked), friction Optional - barriers mutable [] default replaced with None sentinel in both public functions, behavior unchanged - docstring fixes: surface description ("values to bin" leftover), connectivity description, numpy-style spacing 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: Pathfinding API consistency (attrs, Dataset support, type hints, defaults)
Blockers (must fix before merge)
- Adding
@supports_datasettoa_star_searchbreaks keyword calls of the first parameter.a_star_search(surface=agg, start=..., goal=...)worked before this PR and now raisesTypeError: a_star_search() missing 1 required positional argument: 'agg', because the wrapper inxrspatial/dataset_support.py:22isdef wrapper(agg, *args, **kwargs)and only binds the raster positionally. Verified on this branch. The flaw is pre-existing for every other decorated function whose first parameter is not namedagg(cost_distance(raster=...)andproximity(raster=...)fail the same way on main), so the right fix is in the decorator: resolve the first parameter by its real name (from the signature already captured at line 18) when it arrives via kwargs, instead of requiring it positionally. That repairscost_distance/proximitytoo and removes the regression here.
Suggestions (should fix, not blocking)
-
xrspatial/pathfinding.pyReturns section ofa_star_searchstill says only "path_agg: xr.DataArray of the same type assurface". The Parameters entry now documents Dataset input; the Returns entry should mention that a Dataset comes back for Dataset input, the waymulti_stop_search's does.
Nits (optional improvements)
-
test_barriers_default_none_matches_empty_listruns numpy only. Thebarriersnormalization happens before backend dispatch so this is fine, but a one-line comment saying so would save the next reader from wondering.
What looks good
- Every finding in the linked issue #3644 is reproduced in the PR body and covered by a test: attrs preservation, Dataset routing,
None/[]/omitted parity forbarriers. - The attrs merge puts routing keys last, so
waypoint_order/segment_costs/total_costwin over any input attr with the same name. That precedence matches the docstring. - Existing 60-test pathfinding suite passes on a GPU host, including the cupy and dask+cupy tests; the new behavior was also smoke-tested on cupy and dask+cupy directly.
- No kernel or dispatch changes, so no new backend parity or dask chunking surface.
- Sweep state CSV rides along in the same PR, as the sweep contract requires.
Checklist
- Algorithm matches reference/paper (no algorithmic change)
- All implemented backends produce consistent results
- NaN handling is correct (unchanged)
- Edge cases are covered by tests
- Dask chunk boundaries handled correctly (unchanged)
- No premature materialization or unnecessary copies
- Benchmark exists (benchmarks/benchmarks/pathfinding.py, unchanged and still valid: positional call)
- README feature matrix: no new functions, no backend change
- Docstrings present and accurate (see Suggestion on the Returns section)
The wrapper only accepted the raster positionally, so decorating a_star_search regressed a_star_search(surface=...) calls (and cost_distance(raster=...) etc. were already broken the same way on main). The wrapper now resolves the first parameter by its real name when passed as a keyword. Also mention Dataset in the a_star_search Returns section and note why the barriers-default test is numpy-only. 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 follow-up (after 65189ef)
All three findings from the first pass are addressed:
- Blocker (keyword-calling regression): fixed in
xrspatial/dataset_support.py. The wrapper now binds the raster positionally or by its real parameter name, soa_star_search(surface=...)works again, and previously broken keyword calls likecost_distance(raster=...)andproximity(raster=...)work too. Covered bytest_first_param_by_keyword(exercisesslope(agg=),proximity(raster=),a_star_search(surface=)) andtest_first_param_missing_raises_typeerror. - Suggestion (Returns docstring):
a_star_searchReturns section now mentions the Dataset case. - Nit (numpy-only test):
test_barriers_default_none_matches_empty_listnow explains why one backend is enough.
Verified locally: test_dataset_support + test_pathfinding + test_accessor (178 passed) and a broader batch over decorated-function suites (classify, cost_distance, proximity, slope, multispectral: 1113 passed) on a GPU host.
One note for the log, not a request: the second commit's diff to test_pathfinding.py looks larger than it is because the newly appended test block was normalized to the file's CRLF line endings; the content is unchanged apart from the docstring note.
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 #3644
Four small API-consistency fixes in
xrspatial/pathfinding.py, found by /sweep-api-consistency. None of them changes call syntax or results for existing correct code.multi_stop_searchnow keeps the input surface's attrs (crs,res,units, ...) and addswaypoint_order/segment_costs/total_coston top, instead of replacing them.a_star_searchalready preserved attrs; the two siblings now agree.a_star_searchgets@supports_dataset, matchingmulti_stop_searchand the analogous functions elsewhere (cost_distance,proximity,surface_distance, hydro). Addeda_star_searchto the Dataset accessor next to the existingmulti_stop_searchentry.start/goalwere annotatedUnion[tuple, list, np.array](np.arrayis a function, not a type) and are nownp.ndarray;x/ydrop theOptional[str]claim sinceNonewas never accepted;frictionbecomesOptional[xr.DataArray]. Docstring fixes:surfacewas described as "2D array of values to bin" (a copy-paste leftover),connectivityhad no description, and a few parameter entries were missing the numpy-style space before the colon.barriers: list = []mutable defaults in both public functions replaced with aNonesentinel plus in-body substitution. The old default was never mutated, so this is prevention, not a bug fix; a test pinsNone==[]== omitted.Backend coverage: no kernel changes. The full pathfinding suite (60 tests, including the cupy and dask+cupy ones) passes on a GPU host, and the new Dataset/attrs behavior was smoke-tested on cupy and dask+cupy directly.
Test plan:
pytest xrspatial/tests/test_pathfinding.py(60 passed, GPU host)pytest xrspatial/tests/test_accessor.py -k "a_star or multi_stop or expected_methods"(7 passed)a_star_search,barriers=Noneparity,ds.xrs.a_star_searchAlso documented (not fixed): the first parameter is
surfacehere vsraster/aggelsewhere in the library. Naming is split three ways across the codebase, so a local rename would churn a stable signature without settling the convention. Recorded in the sweep state CSV, which is included in this PR.https://claude.ai/code/session_0155N4QGamQVxgpAAPbpQNq4