7
\$\begingroup\$

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) with A a Matrix object. That's why i use res = Matrix(...) in the multiplication 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()
\$\endgroup\$
7
\$\begingroup\$

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.

\$\endgroup\$
  • 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 Feb 12 '18 at 6:24
  • \$\begingroup\$ Thanks, @hjpotter92 , for the res=Matrix(...) i never think that way. \$\endgroup\$ – Arief Anbiya Feb 12 '18 at 11:26
  • \$\begingroup\$ @Arief np. Also, as for the deepcopy/copy, you can define __copy__ and __deepcopy__ methods on Matrix class. \$\endgroup\$ – hjpotter92 Feb 12 '18 at 11:53
7
\$\begingroup\$

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)
\$\endgroup\$
  • \$\begingroup\$ thanks, so that is what @property can do, at least in similar context. But using PyCharm is heavier than the standard IDLE \$\endgroup\$ – Arief Anbiya Feb 12 '18 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\$ – Arief Anbiya Feb 12 '18 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 Feb 12 '18 at 15:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.