35

I've coded my first slightly-complex algorithm, an implementation of the A Star Pathfinding algorithm. I followed some Python.org advice on implementing graphs so a dictionary contains all the nodes each node is linked too. Now, since this is all for a game, each node is really just a tile in a grid of nodes, hence how I'm working out the heuristic and my occasional reference to them.

Thanks to timeit I know that I can run this function successfully a little over one hundred times a second. Understandably this makes me a little uneasy, this is without any other 'game stuff' going on, like graphics or calculating game logic. So I'd love to see whether any of you can speed up my algorithm, I am completely unfamiliar with Cython or it's kin, I can't code a line of C.

Without any more rambling, here is my A Star function.

def aStar(self, graph, current, end):
    openList = []
    closedList = []
    path = []

    def retracePath(c):
        path.insert(0,c)
        if c.parent == None:
            return
        retracePath(c.parent)

    openList.append(current)
    while len(openList) is not 0:
        current = min(openList, key=lambda inst:inst.H)
        if current == end:
            return retracePath(current)
        openList.remove(current)
        closedList.append(current)
        for tile in graph[current]:
            if tile not in closedList:
                tile.H = (abs(end.x-tile.x)+abs(end.y-tile.y))*10 
                if tile not in openList:
                    openList.append(tile)
                tile.parent = current
    return path
DizzyDoo
  • 1,489
  • 6
  • 21
  • 32
  • 14
    `while len(openList) is not 0:` makes me cringe ... `while openlist:` does the same. – Jochen Ritzel Nov 11 '10 at 21:27
  • The line `return retracePath(current)` is incorrect (I think), you shoudl call `retracePath(current)`, then `return path` currently if the end node is found, it returns `None` – monk.e.boy Nov 04 '11 at 22:28

3 Answers3

40

An easy optimization is to use sets instead of lists for the open and closed sets.

openSet   = set()
closedSet = set()

This will make all of the in and not in tests O(1) instead of O(n).

John Kugelman
  • 349,597
  • 67
  • 533
  • 578
  • 1
    Thanks John, second time you've delivered a helpful answer speedily. I've implemented the heapq advice below, but it was using sets that really shaved off the most time (almost a third!). – DizzyDoo Nov 12 '10 at 09:02
10

I would use the sets as have been said, but I would also use a heap to find the minimum element (the one that will be the next current). This would require keeping both an openSet and an openHeap, but the memory shouldn't really be a problem. Also, sets insert in O(1) and heaps in O(log N) so they will be fast. The only problem is that the heapq module isn't really made to use keys with it. Personally, I would just modify it to use keys. It shouldn't be very hard. Alternatively, you could just use tuples of (tile.H,tile) in the heap.

Also, I would follow aaronasterling's idea of using iteration instead of recursion, but also, I would append elements to the end of path and reverse path at the end. The reason is that inserting an item at the 0th place in a list is very slow, (O(N) I believe), while appending is O(1) if I recall correctly. The final code for that part would be:

def retracePath(c):
    path = [c]
    while c.parent is not None:
        c = c.parent
        path.append(c)
    path.reverse()
    return path

I put return path at the end because it appeared that it should from your code.

Here's the final code using sets, heaps and what not:

import heapq


def aStar(graph, current, end):
    openSet = set()
    openHeap = []
    closedSet = set()

    def retracePath(c):
        path = [c]
        while c.parent is not None:
            c = c.parent
            path.append(c)
        path.reverse()
        return path

    openSet.add(current)
    openHeap.append((0, current))
    while openSet:
        current = heapq.heappop(openHeap)[1]
        if current == end:
            return retracePath(current)
        openSet.remove(current)
        closedSet.add(current)
        for tile in graph[current]:
            if tile not in closedSet:
                tile.H = (abs(end.x - tile.x)+abs(end.y-tile.y))*10
                if tile not in openSet:
                    openSet.add(tile)
                    heapq.heappush(openHeap, (tile.H, tile))
                tile.parent = current
    return []
d33tah
  • 10,999
  • 13
  • 68
  • 158
Justin Peel
  • 46,722
  • 6
  • 58
  • 80
  • @hughdbrown, yes, you're not the only one to think of it (pretty standard in pathfinding). I did see that you had thought of it too, but I didn't actually look at your implementation. Personally, I think that a heap should be used and that the tiles should be labeled with an attribute to indicate if each has been visited rather than using sets. However, without doing that my implementation of using both sets and heaps makes the most sense. Also, if he plans on doing multiple runs on these tiles (between different pts) then he would have to reset the visited attribute after each time. – Justin Peel Nov 12 '10 at 04:56
  • 1
    I don't understand how the heap works : the line `heapq.heappush((tile.H,tile))` shouldn't be `heapq.heappush(openHeap,(tile.H,tile))` ?! – Loïc G. Nov 01 '11 at 22:04
  • Yes, you're quite right. I'm not sure how I missed that, but then again it has been almost a year now since I posted this. – Justin Peel Nov 02 '11 at 00:12
  • Yes, but I've this « problem » and your solution gets my attention. Thanks for the reply. – Loïc G. Nov 05 '11 at 17:08
  • I did fix it so I hope that the answer helps. – Justin Peel Nov 06 '11 at 06:02
5

As suggested above, make closedSet into a set.

I tried coding openList as a heap import heapq:

import heapq

def aStar(self, graph, current, end):
    closedList = set()
    path = []

    def retracePath(c):
        path.insert(0,c)
        if c.parent == None:
            return
        retracePath(c.parent)

    openList = [(-1, current)]
    heapq.heapify(openList)
    while openList:
        score, current = openList.heappop()
        if current == end:
            return retracePath(current)
        closedList.add(current)
        for tile in graph[current]:
            if tile not in closedList:
                tile.H = (abs(end.x-tile.x)+abs(end.y-tile.y))*10 
                if tile not in openList:
                    openList.heappush((tile.H, tile))
                tile.parent = current
    return path

However, you still need to search at if tile not in openList, so I would do this:

def aStar(self, graph, current, end):
    openList = set()
    closedList = set()

    def retracePath(c):
        def parentgen(c):
             while c:
                 yield c
                 c = c.parent
        result = [element for element in parentgen(c)]
        result.reverse()
        return result

    openList.add(current)
    while openList:
        current = sorted(openList, key=lambda inst:inst.H)[0]
        if current == end:
            return retracePath(current)
        openList.remove(current)
        closedList.add(current)
        for tile in graph[current]:
            if tile not in closedList:
                tile.H = (abs(end.x-tile.x)+abs(end.y-tile.y))*10 
                openList.add(tile)
                tile.parent = current
    return []
hughdbrown
  • 47,733
  • 20
  • 85
  • 108