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.

• Pierre-Antoine LaFayette: 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.
• magu_: @Pierre-AntoineLaFayette. Very nice link. Thank you I'll look into it.

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

• magu_: 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?
• 200_success: I've rolled back Rev 4 → 3. What you can and cannot do after receiving answers
• magu_: Ah ok. Does this mean I should open a new question with the code and simply link it to this one?
• 200_success: read_and_construct() is still bad, though at least it is honestly named. However, opening the file repeatedly inside zip() is bizarre.
• 200_success: 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.