Hints to make Sudoku solver more Pythonic

Question

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.

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()]