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.

As you will see, I am not very familiar with Python and NumPy but want to learn it.

The following code is a very basic Sudoku solver which works fine for simple examples. Although it runs, I still have the feeling it is not very Pythonic.

from numpy import *
import sys

def set(col,row,wert):
    # Set the value in field and adjust the possibility tensor
    pos[col,row,:] = 0 # the field itself
    pos[:,row,wert] = 0 # the col
    pos[col,:,wert] = 0 # the row

    # The 3x3 block
    col_start = floor(col / 3) * 3
    row_start = floor(row / 3) * 3
    pos[col_start:col_start + 3,row_start:row_start + 3,wert] = 0

    # Write down the value
    feld[col,row] = wert + 1

def read(name):
    # Read in the stuff and create field
    fid = open('in','r')
    for col in range(9):
        line = fid.readline()

        for row in range(9):
            if line[row] is not '0':
                set(row,col,int(line[row]) - 1)
    fid.closed

def write(name):
    # Write the output into a file
    fid = open("out","w")

    for col in range(0,9):
        for row in range(0,9):
            fid.write(str(feld[row,col]))
        fid.write('\n')
    fid.closed

def check():
    # Check the solution for errors
    print("%i Missing" % (81 - count_nonzero(feld)))
    neq = lambda x: count_nonzero(unique(x)) != count_nonzero(x)

    for x in range(9):
        if neq(feld[:,x]):
            print("Error in row %i" % (x))

        if neq(feld[x,:]):
            print("Error in col %i" % (x))

        col_start = floor(x / 3) * 3
        row_start = (x % 3) * 3
        if neq(feld[col_start:col_start + 3,row_start:row_start + 3]):
            print("Error in block %i / %i" % (col_start/3,row_start/3))

def only_pos_left():
    # This point is the only one that can be x
    for x in range(9):
            for y in range(9):          
                if sum(pos[x,:,y]) == 1:
                    set(x,nonzero(pos[x,:,y])[0][0],y)

                if sum(pos[:,x,y]) == 1:
                    set(nonzero(pos[:,x,y])[0][0],x,y)

                col_start = floor(x / 3) * 3
                row_start = (x % 3) * 3

                if sum(pos[col_start:col_start + 3,row_start:row_start + 3,y]) == 1:
                    index = nonzero(pos[col_start:col_start + 3,row_start:row_start + 3,y]);
                    set(col_start + index[0][0],row_start + index[1][0],y)

def only_pos_for_it():
    # This point can only be x
    for x in range(9):
        for y in range(9):
            if sum(pos[x,y,:]) == 1:
                set(x,y,nonzero(pos[x,y,:])[0][0])

# main method
if __name__ == "__main__":
    # Init some variables
    feld = zeros((9,9),dtype=uint8) # field with the values were sure about
    pos = ones((9,9,9),dtype=bool_) # values which are still possible
    old_pos = 0

    read(sys.argv[0])

    # Let it run
    while any(old_pos != pos):
        old_pos = copy(pos)
        only_pos_left()
        only_pos_for_it()

    write(sys.argv[1])
    check()

Some comments on it:

  • I know that from NumPy import * is bad practise but its just so much faster to write.
  • Am I using the lambdas in a correct manner? I want it to be flexible to add more advanced solving algorithms later on.
  • Please tell me just anything you would change to make better use of the language (speed and readability wise).

I know that in terms of data structures this could be optimized to use something like this

class point:
    def __init__(self)
        self.val = uint8
        self.pos = zeros(9,dtype=bool_)

but then I would lose all the nice indexing that NumPy offers.

share|improve this question
1  
Why don't you check out Peter Norvig's famous Sudoku Solver written in Python for inspiration? The implementation is very Pythonic and is interesting code. He also includes a bunch of test cases that can be of interest to you in verifying that your code is working. –  Pierre-Antoine LaFayette Jul 9 at 18:17
    
@Pierre-AntoineLaFayette. Very nice link. Thank you I'll look into it. –  magu_ Jul 10 at 6:02

1 Answer 1

I noticed a few suspicious problems with your coding style. There's plenty to talk about even without analyzing the sudoku solver portion of your program.

Your functions are actually more like procedures that act on global variables. For example, your read() doesn't return anything, and your write() doesn't take a parameter for the matrix to be written. Although set() takes several parameters, the object it mutates is not one of the parameters — it operates on the global pos instead. Similarly, check(), only_pos_left(), and only_pos_for_it() take no parameters at all, which is a bad sign.

Even when read() and write() take a parameter called name (you probably meant "filename"), they ignore the parameter altogether! The filenames 'in' and 'out' are hard-coded.

Your read() and write() each has a file descriptor leak. fid.closed does not close the file handle. You probably meant fid.close(). But really, you should almost always prefer to open files using a with block instead, so you would never need to worry about having to close them again.

Your read() and write() counterintuitively transpose rows ↔︎ columns. I suppose those transpositions cancel each other out, and therefore have no effect. However, it makes it hard for others to think about or discuss your code.

read() tries to do more than just reading the file. The function should do one thing only: return a 2-dimensional matrix. Converting characters to numbers could also be acceptable. However, constructing the data structure you want to use to represent the sudoku puzzle should be a separate function. The fact that read() also subtracts one from each value makes it more disconcerting. Perhaps you do want to have constraints in the 0 to 8 range, but such a transformation definitely doesn't fall under "reading".

To summarize some of the suggestions above…

def read(filename):
    """ Return the contents of the file as a 2-D array. """
    with open(filename) as f:
        return [map(int, row.rstrip()) for row in f.readlines()]
share|improve this answer
    
Thank you very much for your comments. I tried to follow them as good as possible. The thing with global var I def. should have known. The filenames were an artifact from the beginning. You're also totally right about the whole transpose story. I tried to change the read into a for loop style, is that what you meant by it? Is the write fine or should I change it? –  magu_ Jul 9 at 14:29
    
I've rolled back Rev 4 → 3. What you can and cannot do after receiving answers –  200_success Jul 9 at 14:30
    
Ah ok. Does this mean I should open a new question with the code and simply link it to this one? –  magu_ Jul 9 at 14:33
1  
read_and_construct() is still bad, though at least it is honestly named. However, opening the file repeatedly inside zip() is bizarre. –  200_success Jul 9 at 14:33
    
Yes, write a new question and cross-link them. However, I'd suggest waiting a while first to give others a chance to answer, and yourself a chance to think more clearly first. –  200_success Jul 9 at 14:35

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.