Basic beginner's matrix class in Python

Question

This is my first class. I've been working a while on it, some functions are future ideas and are right now just bare bones and intended to be worked on later. I'm really looking for a good review of what I have so far, I've never done this before so hopefully I'm off to a decent start. I tried to log my errors as well and handle them by skipping but annotating which values are trouble. Also first time using *args in my functions.

class Matrix():
 def __init__(self, height, width):
 self.rows = [[0]*width for i in range(height)]
 self.height = height
 self.width = width
 def __str__(self):
 s = "\n" + "\n".join([str(i) for i in [rows for rows in self.rows] ]) + "\n"
 return s
 def __repr__(self):
 return (f'{self.__class__.__name__} ({self.height!r} , {self.width!r})')
 def len(self):
 return self.height * self.width
 def __add__(self, matrix2):
 return
 def __mul__(self, matrix2):
 return
 def remove(self, item):
 return
 def fill_matrix(self, fill_list):
 index = 0
 for i in range(len(self.rows)):
 try:
 for j in range(len(self.rows[i])):
 self.rows[i][j] = fill_list[index]
 index += 1
 except IndexError:
 print (f"Matrix not filled \nMatrix fill stopped at: row {i}, Column {j}")
 break 
 return fill_list[index:]
 def add_value(self, *args):
 log = []
 for arg in args:
 try:
 arg_size = len(arg)
 except TypeError:
 log.append(f'Parameter must be sequence, skipped: {arg}')
 continue
 try:
 if arg_size == 3:
 self.rows[arg[0]] [arg[1]] = arg[2]
 else:
 log.append(f'Parameter has too little or too much data, skipped: {arg}')
 except IndexError:
 log.append(f'Location parameters are out of range, skipped: {arg}') 
 except TypeError:
 log.append(f'Location indicies must contain integral types, skipped: {arg}')
 return log
myMat = Matrix(5,5)
overflow = myMat.fill_matrix([i for i in range(26)])
print(myMat)
Errors = myMat.add_value((-1,3,500), (0,0,3),(51,5, 7), (1, 2, 667), [3,4,676], (1), (1,"a", 1), (1,1, "£"))
print(myMat)

• Anonymous3.1415: I know no one has really said anything about it, but I'm generalizing my fill function to fill partitions or whole thing
• Georgy: Is there any reason not to use NumPy instead of writing your own class?
• Anonymous3.1415: @Georgy, yes, I'm trying to write this Matrix Object to increase my understanding of how to think in an Object Oriented manner and to see if my solution and approach so far is a good start

Answer 1

1. There is no need for brackets in class Matrix(): .... It can be just class Matrix: .... See docs for class definition syntax.
2. In self.rows = [[0]*width for i in range(height)] you should replace i by _ that we usually use for throwaway variables.

3. In the same line 0 is a magic number. You could make it a default parameter:

   def __init__(self, height, width, fill_value=0):
    self.rows = [[fill_value] * width for _ in range(height)]
   

---

4. Regarding __str__:

   def __str__(self):
    s = "\n" + "\n".join([str(i) for i in [rows for rows in self.rows] ]) + "\n"
    return s
   

   Having those "\n" on both ends seems unnecessary. Just return the middle part and let user decide if he wants more blank lines between output.

5. [rows for rows in self.rows] is the same as self.rows. That leaves us:

   def __str__(self):
    s = "\n".join([str(i) for i in self.rows])
    return s
   

6. i is a bad name. row is better.

7. Removing unnecessary s:

   def __str__(self):
    return "\n".join([str(row) for row in self.rows])
   

8. And making it more concise:

   def __str__(self):
    return "\n".join(map(str, self.rows))
   

---

9. Regarding __repr__:

   def __repr__(self):
    return (f'{self.__class__.__name__} ({self.height!r} , {self.width!r})')
   

   The brackets are unnecessary here, and spacing is a bit weird. It should be like this:

   return f'{self.__class__.__name__}({self.height!r}, {self.width!r})'
   

10. As you probably already know, with __repr__ you should be able to pass the returned string to Python interpreter so that it could recreate the object. But in your case information about the values in your matrix would be lost. What you return is just something like Matrix(5, 5). Would this be OK? Probably it would be better if the values would be returned as well, but then the current logic of the class won't allow you to recreate the matrix as you separated filling (fill_matrix) from initializing. Maybe it would be better to be able to do both at the same time?

---

11. len method should be called __len__.

12. As with current implementation its value is constant, you could use lru_cache(1) so to calculate it only once and then reuse the cached value:

    from functools import lru_cache
    ...
    @lru_cache(1)
    def __len__(self) -> int:
     return self.height * self.width
    

13. You have some not implemented yet methods. Instead of returning nothing:

    def __add__(self, matrix2):
     return
    

    raise a NotImplementedError:

    def __add__(self, matrix2):
     raise NotImplementedError
    

---

14. Regarding fill_matrix:

    def fill_matrix(self, fill_list):
     index = 0
     for i in range(len(self.rows)):
     try:
     for j in range(len(self.rows[i])):
     self.rows[i][j] = fill_list[index]
     index += 1
     except IndexError:
     print (f"Matrix not filled \nMatrix fill stopped at: row {i}, Column {j}")
     break 
     return fill_list[index:]
    

    Add a docstring. It is not clear at the first sight why this method should return anything. Something like:

    def fill_matrix(self, fill_list):
     """
     Fills matrix by `fill_list`
     and returns what didn't fit in it.
     """
    

15. You are catching IndexError but this case seems impossible as indices are always taken from the ranges defined by shape of the matrix. You can remove it.

16. Why recalculating len(self.rows) and len(self.rows[i]) if you already have self.height and self.width?

17. The idea of iterating the index doesn't seem Pythonic. How about something like this instead:

    from itertools import product
    ...
    def fill_matrix(self, fill_list):
     fill_values = iter(fill_list)
     indices = product(range(self.height), range(self.width))
     for (i, j), value in zip(indices, fill_values):
     self.rows[i][j] = value
     return fill_values
    

    This can accept iterators or sequences and will return an iterator with "overflow". So if you need to use those leftovers, you get them like this:

    myMat = Matrix(5, 5)
    overflow = myMat.fill(range(30))
    print(list(overflow))
    >>> [25, 26, 27, 28, 29]
    

18. It would be better to rename fill_matrix as fill and its argument fill_list as values.

---

19. Regarding add_value. I think it's better to name it replace instead.

20. This method is too complicated and doesn't follow Single responsibility principle. Catch the errors outside, and use this method just for replacing one value. If you need to replace multiple values, then put the function call inside the loop. See this post for a better explanation. In the end it should be as simple as that:

    def insert(self, row_index, column_index, value):
     self.rows[row_index][column_index] = value
    

    And you could call it, and catch and save the errors like this:

    import traceback
    ...
    values = [(-1, 3, 500),
     (0, 0, 3),
     (51, 5, 7),
     (1, 2, 667),
     [3, 4, 676],
     (1),
     [3, 4, 676, 123],
     (1, "a", 1),
     (1, 1, "£")]
    errors = []
    bad_values = []
    for value in values:
     try:
     myMat.insert(*value)
     except (IndexError, TypeError):
     errors.append(traceback.format_exc())
     bad_values.append(value)
    for error, value in zip(errors, bad_values):
     print(f'Error occurred for value {value}:\n'
     f'{error}')
    

    And the output is:

    Error occurred for value (51, 5, 7):
    Traceback (most recent call last):
     File "C:/Users/Georgy/SO_CR/main.py", line 82, in <module>
     myMat.insert(*value)
     File "C:/Users/Georgy/SO_CR/main.py", line 55, in insert
     self.rows[row_index][column_index] = value
    IndexError: list index out of range
    Error occurred for value 1:
    Traceback (most recent call last):
     File "C:/Users/Georgy/SO_CR/main.py", line 82, in <module>
     myMat.insert(*value)
    TypeError: insert() argument after * must be an iterable, not int
    Error occurred for value [3, 4, 676, 123]:
    Traceback (most recent call last):
     File "C:/Users/Georgy/SO_CR/main.py", line 82, in <module>
     myMat.insert(*value)
    TypeError: insert() takes 4 positional arguments but 5 were given
    Error occurred for value (1, 'a', 1):
    Traceback (most recent call last):
     File "C:/Users/Georgy/SO_CR/main.py", line 82, in <module>
     myMat.insert(*value)
     File "C:/Users/Georgy/SO_CR/main.py", line 55, in insert
     self.rows[row_index][column_index] = value
    TypeError: list indices must be integers or slices, not str
    

    You might be interested in Python logging module as well. Though, I find it strange that you decided to pay so much attention to catching the errors.

---

Some other notes:

• Don't forget about naming conventions. myMat should be my_matrix, Errors should be errors. See PEP 8: for more.
• Consider using type hints.

---

In the end your code could look like this:

from functools import lru_cache
from itertools import product
from typing import (Any,
 Iterable,
 Iterator)
class Matrix:
 def __init__(self,
 height: int,
 width: int,
 fill_value: Any = 0) -> None:
 self.height = height
 self.width = width
 self.rows = [[fill_value] * width for _ in range(height)]
 def __str__(self) -> str:
 return "\n".join(map(str, self.rows))
 def __repr__(self) -> str:
 return f'{self.__class__.__name__}({self.height!r}, {self.width!r})'
 @lru_cache(1)
 def __len__(self) -> int:
 return self.height * self.width
 def __add__(self, matrix2):
 raise NotImplementedError
 def __mul__(self, matrix2):
 raise NotImplementedError
 def remove(self, item):
 raise NotImplementedError
 def fill(self, values: Iterable[Any]) -> Iterator[Any]:
 """
 Fills matrix by `fill_values`
 and returns what didn't fit in it.
 """
 values = iter(values)
 indices = product(range(self.height), range(self.width))
 for (i, j), value in zip(indices, values):
 self.rows[i][j] = value
 return values
 def replace(self,
 row_index: int,
 column_index: int,
 value: Any) -> None:
 """Replaces a value by given indices by a new one."""
 self.rows[row_index][column_index] = value