Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Project Euler problem 83 asks:

In the 5 by 5 matrix below,

131   673 234 103 18
201   96  342 965 150
630   803 746 422 111
537   699 497 121 956
805   732 524 37  331

the minimal path sum from the top left to the bottom right, by moving left, right, up, and down, is

131 → 201 → 96 → 342 → 234 → 103 → 18 → 150 → 111 → 422 → 121 → 37 → 331

and is equal to 2297.

Find the minimal path sum, in matrix.txt (right click and 'Save Link/Target As...'), a 31K text file containing a 80 by 80 matrix, from the top left to the bottom right by only moving right and down.

I have solved the project euler problem 83 using uniform cost search. This solution takes about 0.6s to solve. I want to know if anyone can get the code to run relatively faster without changing the general outline of the program.

import bisect
f = open('matrix.txt')
matrix = [[int(i) for i in j.split(',')] for j in f]

def uniformCostSearch(startNode):
    frontier = []
    frontier.append(startNode)
    closedSet = set()
    while not len(frontier) == 0:
        currentNode = frontier.pop(0)
        if currentNode.x == 79 and currentNode.y == 79:
            return currentNode.priority
        else:
            if not (currentNode.x, currentNode.y) in closedSet:
                closedSet.add((currentNode.x, currentNode.y))
        possibleMoves = currentNode.neighbors()
        for move in possibleMoves:
            if not (move.x, move.y) in closedSet:
                try:
                    index = frontier.index(move)
                    if move.priority < frontier[index].priority:
                        frontier.pop(index)
                        bisect.insort_left(frontier, move)
                except ValueError:
                    # move is not in frontier so just add it
                    bisect.insort_left(frontier, move)
    return -1


class Node:
    def __init__(self, x, y, priority=0):
        self.x = x
        self.y = y
        self.priority = priority

    def neighbors(self):
        tmp = [Node(self.x + 1, self.y), Node(self.x, self.y + 1),
               Node(self.x - 1, self.y), Node(self.x, self.y - 1),]
        childNodes = []
        for node in tmp:
            if node.x >= 0 and node.y >= 0 and node.x <= 79 and node.y <= 79:
                node.priority = self.priority + matrix[node.y][node.x]
                childNodes.append(node)
        return childNodes

    def __eq__(self, node):
        return self.x == node.x and self.y == node.y

    def __ne__(self, node):
        return not self.x == node.x and self.y == node.y

    def __cmp__(self, node):
        if self.priority < node.priority:
            return -1
        elif self.priority > node.priority:
            return 1
        else:
            return 0
share|improve this question
2  
Why not use a classical Dijkstra? –  cat_baxter Nov 27 '12 at 16:40
    
I turned the matrix into a graph and used a shortest path algorithm for this problem which got me the solution in 0.069s. I'll take a look through yours though and see if I can optimize it in anyway. –  Jeremy K Nov 27 '12 at 18:23

1 Answer 1

import bisect
f = open('matrix.txt')
matrix = [[int(i) for i in j.split(',')] for j in f]

You could consider using the numpy library, it'll handle multidimensional arrays more neatly and efficiently.

def uniformCostSearch(startNode):

Python convention is to use underscore_with_letters for function names and parameters

    frontier = []
    frontier.append(startNode)

Use frontier = [startNode], it'll be a touch faster

    closedSet = set()
    while not len(frontier) == 0:

Use while frontier:, it does the same thing and due to less operations is going to be a bit faster. You should at least use != rather then not ==

        currentNode = frontier.pop(0)

Popping from the front of a list will be inefficient. Actually, what you really want is a priority queue, see python's deque module.

        if currentNode.x == 79 and currentNode.y == 79:
            return currentNode.priority
        else:
            if not (currentNode.x, currentNode.y) in closedSet:

Move the not over next to in I think it reads clearer. I might also use a 2D array of bools rather then a set here.

                closedSet.add((currentNode.x, currentNode.y))

There isn't really any reason to check if the node is in the set here, you can add the node multiple times. Just add it unconditionally.

        possibleMoves = currentNode.neighbors()
        for move in possibleMoves:

No reason to store it in a local, just combine the previous two lines

            if not (move.x, move.y) in closedSet:
                try:
                    index = frontier.index(move)
                    if move.priority < frontier[index].priority:
                        frontier.pop(index)
                        bisect.insort_left(frontier, move)

Move the last three lines into an else block. That'll make sure only your index line can actually throw things that'll get caught.

                except ValueError:
                    # move is not in frontier so just add it
                    bisect.insort_left(frontier, move)

You do this in either case, I'd move it after the try/except block. return -1

class Node:
    def __init__(self, x, y, priority=0):
        self.x = x
        self.y = y
        self.priority = priority

    def neighbors(self):
        tmp = [Node(self.x + 1, self.y), Node(self.x, self.y + 1),
               Node(self.x - 1, self.y), Node(self.x, self.y - 1),]
        childNodes = []
        for node in tmp:
            if node.x >= 0 and node.y >= 0 and node.x <= 79 and node.y <= 79:

I suggest moving 79 into a constant. I'd also do < 80 rather then <= 79.

                node.priority = self.priority + matrix[node.y][node.x]
                childNodes.append(node)

I'd use a yield here, and make this a generator

        return childNodes

    def __eq__(self, node):
        return self.x == node.x and self.y == node.y

This bothers me because multiple nodes will compare equal despite having different priorities.

    def __ne__(self, node):
        return not self.x == node.x and self.y == node.y

    def __cmp__(self, node):
        if self.priority < node.priority:
            return -1
        elif self.priority > node.priority:
            return 1
        else:
            return 0

This really bothers me because you aren't defining your comparisons in a consistent way.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.