[qwist1233-cpu]

What

Documents the A* planner in src/components/plan/astar/astar_path_planner.py. Documentation only, no behaviour change: the search, the path file and the GIF come out identical.

• heuristic() gets the equations it implements, f(n) = g(n) + w * h(n) and h(n) = w * (|dx| + |dy|), and an explanation of what the weight buys and what it costs.
• search() gets a step-by-step docstring, each step naming the block of the loop below it: world coordinates to cell indices, the f-ordered heap, the goal test and path reconstruction, g(n') = g(n) + 1 over the 8 neighbours, and the empty-open-list case.
• The constructor docstring listed obstacle_parameters, resolution and visualize, none of which exist, and was missing map_file, path_filename and gif_name. It now lists the real arguments with their units, and notes that x_lim/y_lim are required despite the None default because the cell size is derived from them.

Why the admissibility note

search() charges a cost of 1 for every one of the 8 moves, diagonals included, so the cheapest obstacle-free cost between two cells is the Chebyshev distance max(|dx|, |dy|), not |dx| + |dy|. The Manhattan heuristic therefore overestimates, and an overestimating heuristic is not admissible, so the returned path is not guaranteed to be the shortest. The simulation's weight=5.0 widens that gap.

How it was verified

The numbers in the docstring are measured, not quoted. I re-ran this planner's own search loop (same 8-connected moves, same unit cost, only the heuristic swapped) on the shipped map.json with start = (0, 0) and goal = (50, -10):

heuristic	cells expanded	path cost
Dijkstra, h = 0	7295	100
Manhattan, w = 1.0	538	100
Manhattan, w = 5.0 (the simulation)	121	100
Chebyshev, w = 1.0 (admissible)	3382	100

On this map every variant happens to return an optimal 100-step path, so I did not want to claim suboptimality from the shipped map alone. Over 283 solvable random 30x30 grids with 25% occupied cells, the same loop returned a path longer than Dijkstra's on 219 grids with w = 1.0 (worst case 1.32x) and on 229 with w = 5.0 (worst 1.39x) — the guarantee really is gone, it just does not show up on this particular map.

The ratio in the docstring is exact rather than measured-approximate: the largest value of (|dx| + |dy|) / max(|dx|, |dy|) over the whole 90x120 grid is 2.0, reached on pure diagonals.

Tests: test_astar_path_planning.py passes (72 s), and test_astar_bidirectional_path_planning.py + test_astar_hybrid_path_planning.py pass too (125 s), since they share the components directory. Python 3.12 on Windows.

Closes #102

[qwist1233-cpu]

Gentle nudge on this one — it's docs only (comments and the README section for the A* heuristic weight), so it should be a quick read whenever you have a moment. Happy to rebase or trim it down if you'd rather have a smaller change.

[ShisatoYano]

Hi @qwist1233-cpu, thank you for this PR, and sorry for the slow review!

This is exactly what #102 asked for. The search() steps map cleanly onto the loop, the stale constructor arguments are gone, and the explanation of why Manhattan overestimates on an 8-connected unit-cost grid (up to 2x on pure diagonals) is correct and easy to follow. I re-ran the numbers on map.json and got the same 7295 / 538 / 121 / 3382 expansions with a 100-step path in every case. Thanks for measuring instead of quoting.

Two requests before merging:

1. gif_name after rebasing on main: Show animation by default and save GIFs only with command #160 changed visualize_search(). Now gif_name=None skips the visualization, and a given path shows the animation, or saves it there only when --save-gif is passed. There is no textual conflict, so the current wording would merge silently but be wrong. Could you rebase on main and update it?
2. weight vs. heuristic(): "1.0 is plain A*; a larger value ... gives up the shortest-path guarantee" implies that w = 1.0 keeps the guarantee, but your heuristic() docstring (and your measurement: 219/283 grids at w = 1.0) shows Manhattan is already inadmissible here. Something like "Even with 1.0 the Manhattan heuristic overestimates on this grid, so the shortest path is not guaranteed; a larger w makes the search greedier" would keep the two consistent.

Optional suggestions:

3. The notation for h varies between the constructor, heuristic() and search(). Defining h(n) = |dx| + |dy| once and saying heuristic() returns w*h(n) would make it easier to match the equations to the code.
4. The random-grid statistics can't be reproduced from the docstring (no seed or script). I'd keep the map.json figures there and leave the random-grid numbers in the PR description.
5. path_filename is also effectively required (save_path() calls Path(None) otherwise), so it could get the same note as x_lim/y_lim.

Also, your last comment mentions a README section, but the PR only touches astar_path_planner.py. Was a README change intended?

Thanks again!

[qwist1233-cpu]

Thanks for the careful read — and for re-running the numbers. Rebased on main and pushed; all five points are in.

1. gif_name after #160. Good catch, the old wording would have merged silently wrong. It now reads: None skips the visualization entirely, and with a path the animation is shown with plt.show() and written to that path only when --save-gif is passed (common/save_gif.py).

2. weight vs heuristic(). Fixed the contradiction — the constructor no longer implies 1.0 is safe:

Even at 1.0 that h overestimates the remaining cost on this 8-connected unit-cost grid, so the shortest path is not guaranteed here either; a larger w makes the search greedier and widens the gap.

3. One notation for h. h(n) = |dx| + |dy| is now defined unweighted in all three places, with the weight stated separately: heuristic() says "and this method returns w * h(n)", and search() says f(n) = g(n) + w * h(n), which is what heuristic() returns.

4. Random-grid statistics. Out of the docstring — it keeps the map.json figures only, and points here for the rest. Here they are, reproducible this time (seeded, and the harness is below):

random 30x30, 25% occupied, seed 20261108, 300 draws, 290 solvable
  w=1.0: longer than Dijkstra on 240/290, worst 1.31x
  w=5.0: longer than Dijkstra on 249/290, worst 1.31x

The harness is a matplotlib-free replica of search() — same neighbour order, same uniform cost of 1, same push rule — so it can run headless. Control for the replica: on map.json with start (0, 0), goal (50, -10) it expands 7295 / 538 / 121 cells for h = 0 / w = 1.0 / w = 5.0 and returns a 101-node (100-step) path in all three, i.e. the same figures the real planner prints.

measure_weight.py

import heapq, json
import numpy as np

NB = [(-1, 0), (1, 0), (0, -1), (0, 1), (1, 1), (-1, -1), (1, -1), (-1, 1)]


def search(grid, start_idx, goal_idx, w):
    """Replica of AStarPathPlanner.search(): expansions and path length."""
    rows, cols = grid.shape
    open_list = [(0, start_idx)]
    came_from, cost = {}, {start_idx: 0}
    expanded = 0
    h = lambda a, b: w * (abs(a[0] - b[0]) + abs(a[1] - b[1]))
    while open_list:
        _, cur = heapq.heappop(open_list)
        expanded += 1
        if cur == goal_idx:
            n, path = cur, [cur]
            while n != start_idx:
                n = came_from[n]
                path.append(n)
            return expanded, len(path)
        for dx, dy in NB:
            nb = (cur[0] + dx, cur[1] + dy)
            if 0 <= nb[0] < cols and 0 <= nb[1] < rows and grid[nb[1], nb[0]] == 0:
                nc = cost[cur] + 1
                if nb not in cost or nc < cost[nb]:
                    cost[nb] = nc
                    came_from[nb] = cur
                    heapq.heappush(open_list, (nc + h(nb, goal_idx), nb))
    return expanded, None


# control: the simulation's own map
grid = np.array(json.load(open("src/simulations/path_planning/astar_path_planning/map.json")))
x_min, x_max, y_min = -5, 55, -20
res = (x_max - x_min) / grid.shape[1]
idx = lambda p: (int((p[0] - x_min) / res), int((p[1] - y_min) / res))
s, g = idx((0, 0)), idx((50, -10))
for w in (0.0, 1.0, 5.0):
    print(f"map.json w={w}: {search(grid, s, g, w)}")

# random grids
rng = np.random.default_rng(20261108)
N, SIZE, OCC = 300, 30, 0.25
solvable, longer, worst = 0, {1.0: 0, 5.0: 0}, {1.0: 1.0, 5.0: 1.0}
for _ in range(N):
    gr = (rng.random((SIZE, SIZE)) < OCC).astype(int)
    gr[0, 0] = gr[SIZE - 1, SIZE - 1] = 0
    base = search(gr, (0, 0), (SIZE - 1, SIZE - 1), 0.0)[1]
    if base is None:
        continue
    solvable += 1
    for w in (1.0, 5.0):
        ln = search(gr, (0, 0), (SIZE - 1, SIZE - 1), w)[1]
        if ln > base:
            longer[w] += 1
            worst[w] = max(worst[w], ln / base)
print(f"{solvable} solvable of {N}")
for w in (1.0, 5.0):
    print(f"  w={w}: longer on {longer[w]}/{solvable}, worst {worst[w]:.2f}x")

5. path_filename. Given the same treatment as x_lim/y_lim: "Required as well: once the goal is reached, save_path() calls Path() on it, which raises TypeError on None." (Path(None) → TypeError: argument should be a str or an os.PathLike object....)

On the README: no, that was my mistake in the ping — the PR only ever touched astar_path_planner.py, and I wrote "README section" carelessly. Nothing is missing from the diff. Happy to add a short weighted-A* note to the README in a separate PR if you'd like one, but I'd rather not grow this one.

[ShisatoYano]

Thanks a lot for this, @qwist1233-cpu! The constructor docstring now matches the real signature and behaviour (I checked each argument against the code: the map formats, the resolution derived from x_lim and reused on y, Path(None) raising on a missing path_filename, and the gif_name / --save-gif handling), the admissibility analysis is correct, and the step list in search() maps cleanly onto the loop, including the note about re-expansion. I also re-ran the planner with the four heuristics and got exactly your numbers (7295 / 538 / 121 / 3382 expansions, cost 100 each). Adding Hart et al. and Pohl as references is great too.

One request before merging:

1. search() docstring, f(n) = g(n) + w * h(n), which is what heuristic() returns (L182): heuristic() returns only the w * h(n) term (L162, and your own L122/L160 say so); f is formed in the loop as priority = new_cost + self.heuristic(...) (L234). Since the goal of Explain the A* heuristic and its weight in the A* planner #102 is that a beginner can match each step of the code to an equation, could you reword it, e.g. "f(n) = g(n) + w * h(n); heuristic() returns the w * h(n) term, and the sum is formed where neighbours are pushed"?

Suggestions:

2. The measured paragraph in heuristic() (L136-142): the numbers depend on the obstacles that the simulation hard-codes when it regenerates map.json, so they would silently go stale if the scenario changes, and "figures in the pull request" points readers to something they can't find from the code. I'd keep one qualitative sentence (e.g. "a larger w expands fewer cells but may return a longer path") and drop the numbers. They are a good fit for the Chapter 8 text later.
3. The "not admissible" explanation appears three times (the weight argument L53-58, heuristic() L124-134, search() L199-200). Keeping the full version only in heuristic() and pointing to it from the other two ("see heuristic()") would make the docstrings shorter and easier for beginners to follow.

Minor:

4. "the 90x120 map" (L136) reads better as "90 rows x 120 columns" (moot if the paragraph goes).

🤖 Generated with Claude Code

[qwist1233-cpu]

Thanks for checking every argument against the code. All four are in (33f57c9):

1. search() now reads f(n) = g(n) + w * h(n); heuristic() returns the w * h(n) term and the sum is formed where neighbours are pushed (step 4).
2. The measured paragraph in heuristic() is gone, replaced by one sentence: a larger w expands fewer cells but may return a longer path. Happy to bring the numbers back for the Chapter 8 text later.
3. The full "not admissible" explanation lives only in heuristic(); the weight argument and search() now just point there.
4. Moot with 2.

[ShisatoYano]

Thanks, @qwist1233-cpu! I checked 33f57c9: all four points are addressed. The f(n) line now matches where priority is formed in step 4, the measured paragraph is replaced by the qualitative sentence, and the full admissibility explanation lives only in heuristic(). I also confirmed the PR changes docstrings only (the code is identical to main once docstrings are removed) and CI passes on all platforms. Merging now. Your measurements would be a great fit for Chapter 8 (#203) later, if you'd like to help with that.

🤖 Generated with Claude Code