I have a class
named Matrix
, this class inherits from Python's list
. The class currently can perform matrix multiplication, adding rows using list.extend
, add a row using list.append
, and also transpose.
The emphasis is on using only built-in tools.
- I would like to know if this code can be made more efficient and readable.
- Also, if there are alternative techniques to produce the same result.
- a supplementary problem (more appropriate in StackOverflow) : I cannot use
copy.deepcopy(A)
withA
aMatrix
object. That's why i useres = Matrix(...)
in themultiplication
function.
Thanks.
Here is the code, break into three parts.
Global functions :
# Author: Arief Anbiya
# 11 February, 2018
def dot_product(u, v):
return sum([i*j for i,j in zip(u,v)]
def multiplication(M,N):
assert type(M)==Matrix or type(N)==Matrix
res = None;
if type(M)==type(N)==Matrix and M.ncol()==N.nrow():
res = Matrix([[0 for i in range(N.ncol())] for j in range(M.nrow())]);
for i in range(M.nrow()):
for j in range(N.ncol()):
res[i][j] = dot_product(M[i], [N[k][j] for k in range(N.nrow())]);
elif (type(M)==int or type(M)==float):
res = Matrix([[N[j][i] for i in range(N.ncol())] for j in range(N.nrow())]);
for i in range(res.nrow()):
for j in range(res.ncol()):
res[i][j] *= M;
elif (type(N)==int or type(N)==float):
res = Matrix([[M[j][i] for i in range(M.ncol())] for j in range(M.nrow())]);
for i in range(res.nrow()):
for j in range(res.ncol()):
res[i][j] *= N;
else:
raise TypeError("M and N should be either a compatible Matrix object, or a constant");
return res
Class :
# Author: Arief Anbiya
# 11 February 2018
class Matrix(list):
def __init__(self, the_list):
super().__init__(the_list);
def nrow(self):
m = len(self);
return m
def ncol(self):
n = len(self[0]);
return n
def get_dim(self):
return (self.nrow(),self.ncol())
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
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 for i in range(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');
def transpose(self):
return Matrix([[row[i] for row in self] for i in range(self.ncol())]);
def show(self):
print("Printing matrix: ");
for i in self:
print(i);
Test for outputs :
A=Matrix([[1,2,3], [2,3,3]]);
A.show();
B = A+A;
B.show();
B.append([1,11,12])
B.show();
C = 3*B;
C.show();
D = A*B;
D.show()
2 Answers 2
This is an addendum to Stephen's detailed review
Instead of testing type
of objects, use python's awesome isinstance
function. You can then even have multiple valid types to check against.
assert type(M)==Matrix or type(N)==Matrix
(type(N)==int or type(N)==float)
will simply be:
assert isinstance(M, Matrix) or isinstace(N, Matrix)
isinstance(N, (int, float))
Once you have checked that both \$ M \$ and \$ N \$ are matrices and have computed their multiplication results, you do not need another if-else
block. Just return the result as soon as computed.
Later, store the matrix in one variable and the scalar number in other, and use a single iteration to compute \$ \lambda \cdot A \$.
When creating a null matrix in the following statement:
res = Matrix([[0 for i in range(N.ncol())] for j in range(M.nrow())])
Make use of the \$ * \$ operator on lists:
res = Matrix([[0] * N.ncol for _ in range(M.nrow)]
A rewrite, from my perspective would be:
def multiplication(M, N):
assert isinstance(M, (Matrix, int float)) and isinstace(N, (Matrix, int, float))
if isinstace(M, Matrix) and isinstace(N, Matrix):
if M.ncol != N.nrow
raise TypeError("M and N should be either a compatible Matrix object, or a constant")
res = Matrix([[0] * N.ncol] * M.nrow)
for i in range(M.nrow()):
for j in range(N.ncol):
res[i][j] = dot_product(M[i], [N[k][j] for k in range(N.nrow)])
return res
left, right = (M, N) if isinstace(M, Matrix) else (N, M)
res = Matrix(
[
[left[j][i] * right for i in range(N.ncol)]
for j in range(N.nrow)
]
)
return res
You can have an additional check in the above if you wish to confirm whether left
is also of type Matrix
or not.
-
2\$\begingroup\$
[[0] * N.n col]] * M.nrow
does not do what you think it does. The three inner lists are all the same, so if you modify one element, you modify them all. You have to use[[0] * N.ncol] for _ in range(M.nrow)]
. \$\endgroup\$Graipher– Graipher2018年02月12日 06:24:36 +00:00Commented Feb 12, 2018 at 6:24 -
\$\begingroup\$ Thanks, @hjpotter92 , for the
res=Matrix(...)
i never think that way. \$\endgroup\$Redsbefall– Redsbefall2018年02月12日 11:26:45 +00:00Commented Feb 12, 2018 at 11:26 -
\$\begingroup\$ @Arief np. Also, as for the deepcopy/copy, you can define
__copy__
and__deepcopy__
methods on Matrix class. \$\endgroup\$hjpotter92– hjpotter922018年02月12日 11:53:49 +00:00Commented Feb 12, 2018 at 11:53
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:
- Removes unneeded intermediate values
- 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)
-
\$\begingroup\$ thanks, so that is what
@property
can do, at least in similar context. But using PyCharm is heavier than the standard IDLE \$\endgroup\$Redsbefall– Redsbefall2018年02月12日 11:29:27 +00:00Commented Feb 12, 2018 at 11:29 -
\$\begingroup\$ Thanks again. But do u have any idea why using
copy.deepcopy(A)
result an error..? This is supplementary btw. \$\endgroup\$Redsbefall– Redsbefall2018年02月12日 11:31:28 +00:00Commented Feb 12, 2018 at 11:31 -
\$\begingroup\$ PyCharm takes a bit more, but is pretty amazing. If not PyCharm you can use pylint in many other ways: pylint.readthedocs.io/en/latest/user_guide/ide-integration.html. As to the deepcopy question, I did not get a chance to look at that, instead I constructed the lists of lists directly, which is what deepcopy would have done anyways. GL. \$\endgroup\$Stephen Rauch– Stephen Rauch2018年02月12日 15:33:11 +00:00Commented Feb 12, 2018 at 15:33