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.

I wrote a solution to the Knapsack problem in Python, using a bottom-up dynamic programming algorithm. It correctly computes the optimal value, given a list of items with values and weights, and a maximum allowed weight.

Any critique on code style, comment style, readability, and best-practice would be greatly appreciated. I'm not sure how Pythonic the code is; filling in a matrix seems like a natural way of implementing dynamic programming, but it doesn't "feel" Pythonic (and is that a bad thing, in this case?).

Note that the comments are a little on the verbose side; as I was writing the algorithm, I tried to be as explicit about each step as possible, since I was trying to understand it to the fullest extent possible. If they are excessive (or just plain incorrect), feel free to comment (hah!) on it.

import sys

def knapsack(items, maxweight):
    # Create an (N+1) by (W+1) 2-d list to contain the running values
    # which are to be filled by the dynamic programming routine.
    #
    # There are N+1 rows because we need to account for the possibility
    # of choosing from 0 up to and including N possible items.
    # There are W+1 columns because we need to account for possible
    # "running capacities" from 0 up to and including the maximum weight W.
    bestvalues = [[0] * (maxweight + 1)
                  for i in xrange(len(items) + 1)]

    # Enumerate through the items and fill in the best-value table
    for i, (value, weight) in enumerate(items):
        # Increment i, because the first row (0) is the case where no items
        # are chosen, and is already initialized as 0, so we're skipping it
        i += 1
        for capacity in xrange(maxweight + 1):
            # Handle the case where the weight of the current item is greater
            # than the "running capacity" - we can't add it to the knapsack
            if weight > capacity:
                bestvalues[i][capacity] = bestvalues[i - 1][capacity]
            else:
                # Otherwise, we must choose between two possible candidate values:
                # 1) the value of "running capacity" as it stands with the last item
                #    that was computed; if this is larger, then we skip the current item
                # 2) the value of the current item plus the value of a previously computed
                #    set of items, constrained by the amount of capacity that would be left
                #    in the knapsack (running capacity - item's weight)
                candidate1 = bestvalues[i - 1][capacity]
                candidate2 = bestvalues[i - 1][capacity - weight] + value

                # Just take the maximum of the two candidates; by doing this, we are
                # in effect "setting in stone" the best value so far for a particular
                # prefix of the items, and for a particular "prefix" of knapsack capacities
                bestvalues[i][capacity] = max(candidate1, candidate2)

    # Reconstruction
    # Iterate through the values table, and check
    # to see which of the two candidates were chosen. We can do this by simply
    # checking if the value is the same as the value of the previous row. If so, then
    # we say that the item was not included in the knapsack (this is how we arbitrarily
    # break ties) and simply move the pointer to the previous row. Otherwise, we add
    # the item to the reconstruction list and subtract the item's weight from the
    # remaining capacity of the knapsack. Once we reach row 0, we're done
    reconstruction = []
    i = len(items)
    j = maxweight
    while i > 0:
        if bestvalues[i][j] != bestvalues[i - 1][j]:
            reconstruction.append(items[i - 1])
            j -= items[i - 1][1]
        i -= 1

    # Reverse the reconstruction list, so that it is presented
    # in the order that it was given
    reconstruction.reverse()

    # Return the best value, and the reconstruction list
    return bestvalues[len(items)][maxweight], reconstruction


if __name__ == '__main__':
    if len(sys.argv) != 2:
        print('usage: knapsack.py [file]')
        sys.exit(1)

    filename = sys.argv[1]
    with open(filename) as f:
        lines = f.readlines()

    maxweight = int(lines[0])
    items = [map(int, line.split()) for line in lines[1:]]

    bestvalue, reconstruction = knapsack(items, maxweight)

    print('Best possible value: {0}'.format(bestvalue))
    print('Items:')
    for value, weight in reconstruction:
        print('V: {0}, W: {1}'.format(value, weight))

The input file that it expects is as follows:

165
92 23
57 31
49 29
68 44
60 53
43 38
67 63
84 85
87 89
72 82

The first line contains the maximum weight allowed, and subsequent lines contain the items, represented by value-weight pairs.

share|improve this question
1  
An old post, I know, but if you haven't run into it already enumerate allows you to specify the starting index (e.g. for i,item in enumerate(items,1): would have i begin at 1. –  SimonT Oct 22 '13 at 0:18
    
@SimonT: Fantastic! I've programmed in Python for 2+ years now, and I had never seen enumerate used that way. I'll definitely keep it in mind. –  voithos Oct 22 '13 at 4:44
add comment

1 Answer

up vote 18 down vote accepted

1. Comments on your code

  1. Your function knapsack lacks a docstring that would explain what arguments the function takes (what kind of things are in items? must items be a sequence, or can it be an iterable?) and what it returns.

    Also, this kind of function is ideal for doctests.

    """
    Solve the knapsack problem by finding the most valuable
    subsequence of `items` that weighs no more than `maxweight`.
    
    `items` is a sequence of pairs `(value, weight)`, where `value` is
    a number and `weight` is a non-negative integer.
    
    `maxweight` is a non-negative integer.
    
    Return a pair whose first element is the sum of values in the most
    valuable subsequence, and whose second element is the subsequence.
    
    >>> items = [(4, 12), (2, 1), (6, 4), (1, 1), (2, 2)]
    >>> knapsack(items, 15)
    (11, [(2, 1), (6, 4), (1, 1), (2, 2)])
    """
    
  2. Your comments say things like "Create an (N+1) by (W+1) 2-d list". But what is N and what is W? Presumably N is len(items) and W is maxweight, but this seems needlessly unclear. Better to put a couple of lines like this:

    N = len(items)
    W = maxweight
    

    so that the comments match the code (and then use N and W in the remainder of the code).

  3. The comment above bestvalues fails to explain what the values in this table actually mean. I would write something like this instead:

    # bestvalues[i][j] is the best sum of values for any
    # subsequence of the first i items, whose weights sum
    # to no more than j.
    

    (This makes it obvious why 0 ≤ i ≤ N and why 0 ≤ j ≤ W.)

  4. In a loop like

    bestvalues = [[0] * (maxweight + 1)
                  for i in xrange(len(items) + 1)]
    

    where the loop variable (here i) is unused, it's conventional to name it _.

  5. You can simplify the code by omitting these lines:

    # Increment i, because the first row (0) is the case where no items
    # are chosen, and is already initialized as 0, so we're skipping it
    i += 1
    

    and then using i + 1 instead of i and i instead of i - 1.

  6. Your reconstruction loop:

    i = N
    while i > 0:
        # code
        i -= 1
    

    can be written like this:

    for i in xrange(N, 0, -1):
        # code
    
  7. You print an error message like this:

    print('usage: knapsack.py [file]')
    

    Error messages ought to go to standard error (not standard output). And you can't know that your program is called "knapsack.py": it might have been renamed. So write instead:

    sys.stderr.write('usage: {0} [file]\n'.format(sys.argv[0]))
    
  8. Your block of code that reads the problem description and prints the result only runs when __name__ == '__main__'. This makes it hard to test, for example from the interactive interpreter. It's usually best to put this kind of code in its own function, like this:

    def main(filename):
        with open(filename) as f:
            # etc.
    
    if __name__ == '__main__':
        if len(sys.argv) != 2:
            print('usage: knapsack.py [file]')
            sys.exit(1)
        main(sys.argv[1])
    

    and now you can run main('problem.txt') from the interpreter to test it.

  9. You read the whole of the file into memory as a list of lines:

    lines = f.readlines()
    

    this is harmless here because the file is small, but it's a bad habit to get into. It's usually best to process a file one line at a time if you can, like this:

    with open(filename) as f:
        maxweight = int(next(f))
        items = [map(int, line.split()) for line in f]
    

    (Note that this results in slightly simpler code too.)

2. A more Pythonic solution?

Any dynamic programming algorithm can be implemented in two ways: by building a table of partial results from the bottom up (as in your code), or by recursively computing the result from the top down, using memoization to avoid computing any partial result more than once.

There are two advantages of the top-down approach: first, it often results in slightly simpler and clearer code, and second, it only computes the partial results that are needed for the particular problem instance (whereas the bottom-up approach computes all partial results even if some of them go unused).

So we could use the @memoized decorator from the Python Decorator Library to implement a top-down solution, as shown below. (The Python wiki seems to be down at the moment, but you can find the code on archive.org.)

def knapsack(items, maxweight):
    """
    Solve the knapsack problem by finding the most valuable
    subsequence of `items` subject that weighs no more than
    `maxweight`.

    `items` is a sequence of pairs `(value, weight)`, where `value` is
    a number and `weight` is a non-negative integer.

    `maxweight` is a non-negative integer.

    Return a pair whose first element is the sum of values in the most
    valuable subsequence, and whose second element is the subsequence.

    >>> items = [(4, 12), (2, 1), (6, 4), (1, 1), (2, 2)]
    >>> knapsack(items, 15)
    (11, [(2, 1), (6, 4), (1, 1), (2, 2)])
    """

    # Return the value of the most valuable subsequence of the first i
    # elements in items whose weights sum to no more than j.
    @memoized
    def bestvalue(i, j):
        if i == 0: return 0
        value, weight = items[i - 1]
        if weight > j:
            return bestvalue(i - 1, j)
        else:
            return max(bestvalue(i - 1, j),
                       bestvalue(i - 1, j - weight) + value)

    j = maxweight
    result = []
    for i in xrange(len(items), 0, -1):
        if bestvalue(i, j) != bestvalue(i - 1, j):
            result.append(items[i - 1])
            j -= items[i - 1][1]
    result.reverse()
    return bestvalue(len(items), maxweight), result

To see how many partial solutions this code needs to compute, print len(bestvalue.cache) just before returning the result. When solving the example problem in the docstring, I find that this computes 37 partial solutions (compared to the 96 partial solutions computed by the bottom up approach).

share|improve this answer
    
Thanks for your response (especially points 3 and 9)! I can't believe I forgot to consider the memoized recursive method. I'll apply some of your suggestions later today, hopefully. Thanks again. –  voithos Jan 16 '13 at 22:19
    
Beware of recursion depth limit issues with this proposed solution. –  Frank Smith Mar 8 at 15:13
    
@Frank: yes, this solution uses len(items) levels of stack. –  Gareth Rees Mar 13 at 8:26
add comment

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.