Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

###Iterate directly

Iterate directly

###zip() is transpose

zip() is transpose

###@property make for a cleaner interface

@property make for a cleaner interface

###Full Listing

Full Listing

###Iterate directly

###zip() is transpose

###@property make for a cleaner interface

###Full Listing

Iterate directly

zip() is transpose

@property make for a cleaner interface

Full Listing

Source Link
Stephen Rauch
  • 4.3k
  • 12
  • 24
  • 36

This review will focus on using Python more effectively. I will pull some examples from your Matrix class, and show those recast in a more Pythonic fashion.

Pep8

But before we start, you should consider formatting your code in accordance with pep8. This is important when sharing code, as the consistent style makes it much easier for other programmers to read your code. There are various tools available to assist in making the code pep8 compliant. I use the PyCharm IDE which will show pep8 violations right in the editor.

No Semi Colons

On the same front, lose the semi colons, they are very rarely (never really) needed in Python.

###Iterate directly

Python has many great ways to iterate. In general if you find yourself looping on an integer, like in the following code, there is a good chance you are doing it wrong. It is generally better to iterate on the data structure itself. This example:

def __add__(self, M):
 res = Matrix(self);
 for row in range(self.nrow()):
 dumrow = [self[row][col] + M[row][col] for col in
 range(self.ncol())];
 res[row] = dumrow;
 return res
 

Can be reduced to:

def __add__(self, M):
 return Matrix([[sum(x) for x in zip(*rows)] for rows in zip(self, M)])
 

Note that the lengths of the rows or columns are not even looked at in this code. The underlying iterators take care of those details.

###zip() is transpose

This hand coded transpose:

def transpose(self):
 return Matrix(
 [[row[i] for row in self] for i in range(self.ncol())]); 

can be recast to:

@property
def transpose(self):
 return Matrix(map(list, zip(*self))) 
 

zip() does the transpose, and the map(list(...)) converts the tuples from zip to lists. And...

###@property make for a cleaner interface

Instead of:

def nrow(self):
 m = len(self);
 return m

Consider this instead:

@property
def nrow(self):
 return len(self)

It does two things:

  1. Removes unneeded intermediate values
  2. Use the @property decorator to allow calculated value to be returned without the need for the (). This can provide a cleaner api.

###Full Listing

class Matrix(list):
 def __init__(self, the_list):
 super().__init__(the_list)
 @property
 def nrow(self):
 return len(self)
 @property
 def ncol(self):
 return len(self[0])
 def get_dim(self):
 return self.nrow, self.ncol
 def __add__(self, M):
 return Matrix(
 [[sum(x) for x in zip(*rows)] for rows in zip(self, M)])
 def __mul__(self, M):
 return multiplication(self, M)
 def __rmul__(self, M):
 return multiplication(M, self)
 def add_rows(self, rows):
 super().extend(rows)
 def append(self, row):
 try:
 sum(row) # Check if all numbers
 if len(row) < self.ncol:
 row.extend([0] * (self.ncol - len(row)))
 elif len(row) > self.ncol:
 [row.pop() for i in range(len(row) - self.ncol)]
 super().append(row)
 except:
 raise AssertionError('Elements in row must be mumbers')
 @property
 def transpose(self):
 return Matrix(map(list, zip(*self)))
 def show(self):
 print("Printing matrix: ")
 for i in self:
 print(i)
lang-py

AltStyle によって変換されたページ (->オリジナル) /