5
\$\begingroup\$

I have written a matrix class using python, allowing me to perform a variety of operations on matrices. I'm pretty happy with how it performs, and everything works fine. However, I feel like the way I've handled the initialiser could be much improved.

I wasn't sure how to handle different possible cases for the arguments passed to the __init__ method, due to the lack of multiple constructors. I want to at least support the creation of a matrix from a list of lists, or the number of rows and columns. What I ended up going with was using isinstance() to check if the arguments are of a certain type, and handle each case that way.

Would it be better to use different class methods for each case, or is an approach similar to my own using isinstance() considered appropriate?

I would also appreciate a general overview of the class, to see what aspects of it could be improved.

from copy import deepcopy


class MatrixError(Exception):
    """An exception class for Matrix"""
    pass


class Matrix:
    matrix = []

    def __init__(self, *args):
        if len(args) == 1:
            if isinstance(args[0], Matrix):
                self.matrix = deepcopy(args[0].matrix)

                self.rows = len(self.matrix)
                self.cols = len(self.matrix[0])

            elif isinstance(args[0], list):
                self.rows = len(args[0])
                self.cols = len(args[0][0])

                if all(map(lambda x: len(x) == self.cols, args[0])):
                    self.matrix = deepcopy(args[0])
                else:
                    raise MatrixError("Uneven columns")

            else:
                raise MatrixError("Expected Matrix, list, or int * int")

        elif len(args) == 2):
            if isinstance(args[0], int) and isinstance(args[1], int):
                self.rows = args[0]
                self.cols = args[1]
                self.matrix = [[0] * args[1] for row in range(args[0])]

            else:
                raise MatrixError("Expected Matrix, list, or int * int")

        else:
            raise MatrixError("Expected Matrix, list, or int * int")

    @classmethod
    def identity(cls, size):
        new_matrix = cls(size, size)

        for i in range(size):
            new_matrix[i][i] = 1

        return new_matrix

    def __str__(self):
        return str(self.matrix)

    def __getitem__(self, key):
        return self.matrix[key]

    def __eq__(self, other):
        if isinstance(other, Matrix):
            if self.rows != other.rows:
                return False  # They don't have the same dimensions, they can't be equal

            # Test if all rows are equal
            return all(map(lambda rows: rows[0] == rows[1], zip(self.matrix, other.matrix)))

        else:
            return False

    def __ne__(self, other):
        return not self.__eq__(other)  # Check for equality and reverse the result

    def __pos__(self):
        return self

    def __neg__(self):
        return -1 * self

    def __add__(self, other):
        new_matrix = Matrix(self.rows, self.cols)

        if isinstance(other, Matrix):
            if (self.rows == other.rows) and (self.cols == other.cols):
                for row in range(self.rows):
                    for column in range(self.cols):
                        new_matrix[row][column] = self[row][column] + other[row][column]

            else:
                raise MatrixError("Can't add or subtract %d x %d matrix with %d x %d matrix" %
                                  (self.rows, self.cols, other.rows, other.cols))
        else:
            return NotImplemented

        return new_matrix

    def __sub__(self, other):
        return self + (other * -1)

    def __mul__(self, other):
        if isinstance(other, (int, float, complex)):
            new_matrix = Matrix(self.rows, self.cols)

            for row in range(self.rows):
                for col in range(self.cols):
                    new_matrix[row][col] = self[row][col] * other

        elif isinstance(other, Matrix):
            if self.cols == other.rows:
                new_matrix = Matrix(self.rows, other.cols)

                for row in range(self.rows):
                    for col in range(other.transpose().rows):
                        value = 0
                        for i in range(self.cols):
                            value += self[row][i] * other[i][col]

                        new_matrix[row][col] = value

            else:
                raise MatrixError("Can't multiply (%d, %d) matrix with (%d, %d) matrix" %
                                  (self.rows, self.cols, other.rows, other.cols))

        else:
            return NotImplemented

        return new_matrix

    def __rmul__(self, other):
        return self.__mul__(other)

    def transpose(self):
        new_matrix = Matrix(self.cols, self.rows)

        for row in range(self.rows):
            for col in range(self.cols):
                new_matrix[col][row] = self[row][col]

        return new_matrix

    def is_square(self):
        return self.rows == self.cols

    def minor_matrix(self, remove_row, remove_col):
        new_matrix_array = [row[:remove_col] + row[remove_col + 1:] for row in (self[:remove_row] + self[remove_row + 1:])]
        return Matrix(new_matrix_array)

    def determinant(self):
        if self.is_square():
            # base case for 2x2 matrix
            if self.rows == 2:
                return self[0][0] * self[1][1] - self[0][1] * self[1][0]

            determinant = 0
            for col in range(self.cols):
                determinant += ((-1) ** col) * self[0][col] * self.minor_matrix(0, col).determinant()

            return determinant
        else:
            raise MatrixError("Determinant only defined for square matrices")

    def inverse(self):
        if self.is_square():
            determinant = self.determinant()
            if determinant == 0:
                raise MatrixError("Matrix is singular")

            # special case for 2x2 matrix:
            if self.rows == 2:
                return [[self[1][1] / determinant, -1 * self[0][1] / determinant],
                        [-1 * self[1][0] / determinant, self[0][0] / determinant]]

            # find matrix of minors
            minors = Matrix(self.rows, self.cols)

            for row in range(self.rows):
                for col in range(self.cols):
                    minor = self.minor_matrix(row, col)
                    minors[row][col] = ((-1) ** (row + col)) * minor.determinant()

            t_minors = minors.transpose()

            for row in range(t_minors.rows):
                for col in range(t_minors.cols):
                    t_minors[row][col] /= determinant

            return t_minors

        else:
            raise MatrixError("Inverse only defined for square matrices")
\$\endgroup\$
2
\$\begingroup\$

Just a quick reminder before we get started: In case you want to do anything serious with matrices in Python, consider using the NumPy library instead of roling your own matrix class. NumPy is the de-facto standard when it comes to numeric operations in Python and is in the general case (very likely much) faster than anything you write yourself. It's always the loops that get you in the end ;-)


Overall feedback

Your code looks quite good from an overall point of you regarding style and layout. The Matrix class would profit from further documentation using docstrings, such as the one you have started to use on MatrixError. This is especially important when you have methods/constructors that accept *args. Without proper documentation, everything that remains for someone else but you would be to read the code or get it working by trial and error.

Class interface

I would consider declaring self.matrix as "private"/internal by prefixing it with an _ to discourage user to directly interact with the inner data representation of your class. To allow changing the data, consider also implementing __setitem__ for your class. Having classmethods to construct special matrices is quite a common approach, e.g. used by the C++ libraries Eigen (e.g. MatrixXf::Identity) and OpenCV (e.g. Mat::eye). NumPy on the other hand has them as top-level module functions (e.g np.identity). You'll have to decide what's your preferred way to go.

The constructor

The "copy" constructor looks a little bit odd to me. Why would you bother using self.rows = len(self.matrix) and self.cols = len(self.matrix[0]) when the other matrix should have the attributes properly set? Dont't you trust your own implementation? ;-)

The same error message is repeated three times in the constructor. Under the assumption you wouldn't want to have a zero-sized matrix (no rows, no cols, no data), removing them and checking

if not self.matrix:
    raise MatrixError("Expected Matrix, list, or int * int")

at the end will help you to reduce the duplication here. Maybe also use None instead of [] as default value of self.matrix and then check for if self.matrix is None:.

I'm also not entirely sure "int * int" gets the message across to possible users, so that they really understand what they are supposed to provide to the constructor.

Reduce nesting

Some of your methods like __eq__, __add__, determinant, inverse perform input validation and raise an exception if the input is not valid. You can reduce the level of nesting in that case if you return early. So instead of

def determinant(self):
    if self.is_square():
        ... # do the actual work
    else:
        raise MatrixError("Determinant only defined for square matrices")

do

def determinant(self):
    if self.is_square():
        raise MatrixError("Determinant only defined for square matrices")
    ... # do the actual work

This helps to reduce the needed level of indentation and makes it clearer what happens when the condition is violated.

String formatting

A more modern approach to generate the error message in __add__ and __mul__ would either be to use .format(...) (Python 2, Python 3) or f-strings (Python 3.6+):

# .format(...)
MatrixError(
    "Can't multiply ({}, {}) matrix with ({}, {}) matrix".format(
        self.rows, self.cols, other.rows, other.cols
    )
)

# f-string
MatrixError(
    f"Can't add or subtract {self.rows} x {self.cols} matrix "
    f"with {other.rows} x {other.cols} matrix"
)

Performance

The implementation of __sub__ is computationally wasteful, since you have to iterate over all the rows and columns twice. Although it does not get rid of that problem, you can also implement this using your __neg__ as -other + self.

|improve this answer|||||
\$\endgroup\$
3
\$\begingroup\$

A typo

elif len(args) == 2): closes an unopened parenthesis.

An unnecessary transpose

In __mul__, this seems strange:

for col in range(other.transpose().rows):

This would transpose the other matrix lots of times, just to access the row count of the transposes. Why not:

for col in range(other.cols):

Slow inverse and determinant

The cofactor expansion algorithm works well for small matrixes, but suffers severe performance degradation as the matrix grows. The time complexity of cofactor expansion is O(n!), that really limits the ability to work with matrixes beyond "tiny", the biggest I could go without running out of patience was 8x8. Both the determinant and the inverse (if it exists) can be found in O(n³) time by LU-factoring the matrix.

|improve this answer|||||
\$\endgroup\$

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.