feat: add astar_reach for steppable A* search - #787
Conversation
78a1aa5 to
41c1039
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Inconsistent or stateful heuristics can produce incorrect costs and misleading expansion priorities.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a steppable A* iterator for inspecting node expansions and stopping externally.
Changes:
- Introduces
astar_reachand its iterator/item types. - Adds coverage for costs, expansion behavior, parents, and fused iteration.
File summaries
| File | Description |
|---|---|
src/directed/astar.rs |
Implements the A* reachability iterator and public API. |
tests/astar-reach.rs |
Tests iterator behavior and comparison with existing searches. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if !self.seen.insert(index) { | ||
| continue; |
| let item; | ||
| let successors = { | ||
| let (node, &(parent_index, _)) = self.parents.get_index(index).unwrap(); | ||
| let estimated_cost = total_cost + (self.heuristic)(node); |
A* could not be interrupted or inspected mid-search. astar_reach yields each expansion so callers can step, visualize, or stop early. Co-authored-by: Cursor <cursoragent@cursor.com>
… cheaper path appears Two problems raised in review, both of which made the iterator disagree with `astar` on graphs `astar` handles. A closed set stopped a node being expanded a second time. `astar` requires only that the heuristic be admissible, and an admissible heuristic need not be consistent: when it drops by more than the cost of the edge travelled, a cheaper route to an already-expanded node turns up later and `astar` expands it again. Suppressing that does not merely hide the repeat, it reports costs that are wrong. On the graph from the review, with `S->A` 3, `S->B` 1, `B->A` 1, `A->G` 1 and `h(A)` 0 against `h(B)` 2, the iterator gave the goal a cost of 4 where `astar` returns 3. A node is now yielded again when a cheaper path to it is found, which is what `astar` does, and the documentation says so instead of promising each node once. `estimated_cost` was recomputed by calling the heuristic again for the node being expanded, rather than reported from the queue entry that selected it. The heuristic is an `FnMut` and may be stateful, so the extra call both reports a value that took no part in the search and perturbs the search being watched. The popped priority is now carried through. This also fixes the start node, which was queued with an estimate of zero rather than its own heuristic; nothing read that before, because expansion recomputed it. Both are covered by tests that fail against the previous code: the review's graph, checked against `astar` and asserting the node really is expanded twice, and a heuristic that answers differently the second time it is asked about a node.
41c1039 to
1be7e83
Compare
|
Both review comments are addressed, and the branch is rebased onto current Re-expansionThe report is right, and the counterexample reproduces exactly: on I took the first of the two options offered — support re-expansion — rather than requiring a consistent heuristic, for two reasons. The closed set is gone. A node is yielded again when a cheaper path to it appears, exactly as
|
Summary
Implements
astar_reachfor #775.A* currently runs to completion. There is no way to step it, inspect expansions, or stop early.
dijkstra_reach,bfs_reach, anddfs_reachalready provide that for the other searches.astar_reach(start, successors, heuristic)yields oneAstarReachableItemper expansion:node,parent,total_cost(g) — same idea asDijkstraReachableItemestimated_cost(f = g + h) — so a visualizer can show the A* priorityNodes come out in the same order
astarwould expand them. Each node is yielded at most once. There is no goal predicate; stop from the outside withfind,take_while, or by dropping the iterator.idastar/iddfsare not in this PR. They restart the search at each bound, so a useful iterator is a different design.Closes #775
Test plan
cargo test --test astar-reachdijkstra_reachon the same graphastarh = 0