From 18f5c759d3fc774280ee206f50ed7f1ef70ad2d1 Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Wed, 8 Jul 2026 17:32:35 -0400 Subject: [PATCH 1/2] Reuse symmetric costs in optimize_order instead of running reverse A* (#3661) _optimize_waypoint_order ran a_star_search for every ordered waypoint pair, N(N-1) searches, even though the cost matrix is symmetric (the pixel graph is undirected and edge costs use the mean friction of both endpoints). The module header already described the intended N(N-1)/2 behavior. The matrix build now mirrors cost(i->j) into cost(j->i) and only runs the upper triangle, halving the dominant cost of optimize_order=True. With snap=True both directions still run, since snapping can move the requested pixels and break the symmetry of the read-out pixel. Adds call-count regression tests for both the snap-off and snap-on cases. Claude-Session: https://claude.ai/code/session_0155N4QGamQVxgpAAPbpQNq4 --- xrspatial/pathfinding.py | 43 +++++++++++++-------- xrspatial/tests/test_pathfinding.py | 60 +++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 16 deletions(-) diff --git a/xrspatial/pathfinding.py b/xrspatial/pathfinding.py index 3c1a4199c..bf9e0c24a 100644 --- a/xrspatial/pathfinding.py +++ b/xrspatial/pathfinding.py @@ -1330,23 +1330,34 @@ def _optimize_waypoint_order(surface, waypoints, barriers, x, y, INF = float('inf') dist = [[INF] * n for _ in range(n)] + def _pair_cost(a, b): + seg = a_star_search( + surface, waypoints[a], waypoints[b], + barriers=barriers, x=x, y=y, + connectivity=connectivity, + snap_start=snap, snap_goal=snap, + friction=friction, search_radius=search_radius, + ) + seg_vals = _segment_to_numpy(seg.data) + goal_py, goal_px = _get_pixel_id(waypoints[b], surface, x, y) + goal_cost = seg_vals[goal_py, goal_px] + return float(goal_cost) if np.isfinite(goal_cost) else INF + for i in range(n): - for j in range(n): - if i == j: - dist[i][j] = 0.0 - continue - seg = a_star_search( - surface, waypoints[i], waypoints[j], - barriers=barriers, x=x, y=y, - connectivity=connectivity, - snap_start=snap, snap_goal=snap, - friction=friction, search_radius=search_radius, - ) - seg_vals = _segment_to_numpy(seg.data) - goal_py, goal_px = _get_pixel_id(waypoints[j], surface, x, y) - goal_cost = seg_vals[goal_py, goal_px] - if np.isfinite(goal_cost): - dist[i][j] = goal_cost + dist[i][i] = 0.0 + for j in range(i + 1, n): + dist[i][j] = _pair_cost(i, j) + if snap: + # Snapping can move the requested start and goal pixels, + # so the reverse direction may read its cost at a + # different pixel; compute it explicitly. + dist[j][i] = _pair_cost(j, i) + else: + # The pixel graph is undirected and edge costs use the + # mean friction of both endpoints, so the cost matrix is + # symmetric: reuse cost(i -> j) as cost(j -> i) instead + # of running the reverse A* search. + dist[j][i] = dist[i][j] # Fixed endpoints: first=0, last=n-1 if n <= 12: diff --git a/xrspatial/tests/test_pathfinding.py b/xrspatial/tests/test_pathfinding.py index 0db74e791..f99f61dd2 100644 --- a/xrspatial/tests/test_pathfinding.py +++ b/xrspatial/tests/test_pathfinding.py @@ -1062,6 +1062,66 @@ def test_optimize_order_preserves_endpoints(): assert tuple(order[-1]) == wp3 +def test_optimize_order_symmetric_matrix_call_count(): + """Cost matrix reuses symmetric costs: N(N-1)/2 A* runs, not N(N-1). + + Issue #3661: the pixel graph is undirected and edge costs use the + mean friction of both endpoints, so cost(i->j) == cost(j->i) and + the reverse searches are redundant when snap is off. + """ + import xrspatial.pathfinding as pf + + data = np.ones((10, 10)) + agg = _make_raster(data) + + wps = [(9.0, 0.0), (5.0, 5.0), (5.0, 0.0), (9.0, 9.0), (0.0, 9.0)] + n = len(wps) + + calls = [] + orig = pf.a_star_search + + def counting(*args, **kwargs): + calls.append(1) + return orig(*args, **kwargs) + + from unittest.mock import patch + with patch.object(pf, 'a_star_search', side_effect=counting): + order = pf._optimize_waypoint_order( + agg, wps, [], 'x', 'y', 8, False, None, None) + + assert len(calls) == n * (n - 1) // 2 + # endpoints stay fixed and all waypoints are kept + assert order[0] == wps[0] + assert order[-1] == wps[-1] + assert sorted(order) == sorted(wps) + + +def test_optimize_order_snap_keeps_both_directions(): + """With snap=True the reverse searches still run (snapping can move + the requested pixels, so symmetry is not guaranteed).""" + import xrspatial.pathfinding as pf + + data = np.ones((10, 10)) + agg = _make_raster(data) + + wps = [(9.0, 0.0), (5.0, 5.0), (0.0, 9.0)] + n = len(wps) + + calls = [] + orig = pf.a_star_search + + def counting(*args, **kwargs): + calls.append(1) + return orig(*args, **kwargs) + + from unittest.mock import patch + with patch.object(pf, 'a_star_search', side_effect=counting): + pf._optimize_waypoint_order( + agg, wps, [], 'x', 'y', 8, True, None, None) + + assert len(calls) == n * (n - 1) + + # --- Cross-backend tests --- @pytest.mark.skipif(not has_dask_array(), reason="Requires dask.Array") From 6dd3ea71c349be2f21f79f7bd66e1ce854b859e7 Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Wed, 8 Jul 2026 17:38:49 -0400 Subject: [PATCH 2/2] Address review: exercise real snap relocation in the both-directions test (#3661) The middle waypoint now sits on a NaN cell so snapping actually moves it. The relocated waypoint can be dropped from the returned order; that is pre-existing behavior tracked in #3646 and noted in the test. Claude-Session: https://claude.ai/code/session_0155N4QGamQVxgpAAPbpQNq4 --- xrspatial/tests/test_pathfinding.py | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/xrspatial/tests/test_pathfinding.py b/xrspatial/tests/test_pathfinding.py index f99f61dd2..26e263ae7 100644 --- a/xrspatial/tests/test_pathfinding.py +++ b/xrspatial/tests/test_pathfinding.py @@ -1096,15 +1096,25 @@ def counting(*args, **kwargs): assert sorted(order) == sorted(wps) +@pytest.mark.filterwarnings("ignore:Start at a non crossable location:Warning") +@pytest.mark.filterwarnings("ignore:End at a non crossable location:Warning") def test_optimize_order_snap_keeps_both_directions(): """With snap=True the reverse searches still run (snapping can move - the requested pixels, so symmetry is not guaranteed).""" + the requested pixels, so symmetry is not guaranteed). + + The middle waypoint sits on a NaN cell so snapping really relocates + it: the direction ending at that waypoint reads its cost at the + requested (NaN) pixel while the reverse starts from the snapped + pixel, which is the asymmetry the double computation protects. + """ import xrspatial.pathfinding as pf data = np.ones((10, 10)) + data[5, 5] = np.nan # snap target: waypoint below lands here agg = _make_raster(data) - wps = [(9.0, 0.0), (5.0, 5.0), (0.0, 9.0)] + # coords: y=[9..0], x=[0..9]; (4.0, 5.0) maps to pixel (5, 5) + wps = [(9.0, 0.0), (4.0, 5.0), (0.0, 9.0)] n = len(wps) calls = [] @@ -1116,10 +1126,17 @@ def counting(*args, **kwargs): from unittest.mock import patch with patch.object(pf, 'a_star_search', side_effect=counting): - pf._optimize_waypoint_order( + order = pf._optimize_waypoint_order( agg, wps, [], 'x', 'y', 8, True, None, None) assert len(calls) == n * (n - 1) + assert order[0] == wps[0] + assert order[-1] == wps[-1] + # Note: the NaN waypoint may be missing from the returned order. + # That is pre-existing behavior tracked in issue #3646 (waypoints + # silently dropped when the matrix has no finite tour), not part + # of the call-count contract under test here. + assert set(order) <= set(wps) # --- Cross-backend tests ---