Exercism: Class representation of a matrix in Python

Question

The task is to represent a Matrix as a class and to instantiate it from a string such as "1 2 3 4\n5 6 7 8\n9 8 7 6" The class must provide methods for accessing individual rows and columns by index starting at 1.
While my solution is relatively short and more or less clear, I'm wondering about the ways to make it better.

One thing I'm not sure about is calling _get_rows() in different places inside the class, instead of assigning its result to class attribute.

Here is my code.

class Matrix:
 """Class representation of a matrix""" 
 
 def __init__(self, matrix_string):
 self.matrix_string = matrix_string
 def _get_rows(self):
 """Produce a list of rows of the matrix."""
 
 rows = []
 for row_string in self.matrix_string.split('\n'):
 rows.append([int(char) for char in row_string.split()])
 
 return rows 
 
 
 def _get_columns(self):
 """Produce a list of columns of the matrix."""
 
 rows = self._get_rows()
 
 return [list(row) for row in zip(*rows)]
 
 
 def row(self, index):
 """Produce i-th row of the matrix with first index being 1"""
 
 return self._get_rows()[index - 1]
 def column(self, index):
 """Produce i-th column of the matrix with first index being 1"""
 
 return self._get_columns()[index - 1]

And the unit tests provided by exercism.

import unittest
from matrix import Matrix
# Tests adapted from `problem-specifications//canonical-data.json` @ v1.3.0
class MatrixTest(unittest.TestCase):
 def test_extract_row_from_one_number_matrix(self):
 matrix = Matrix("1")
 self.assertEqual(matrix.row(1), [1])
 def test_can_extract_row(self):
 matrix = Matrix("1 2\n3 4")
 self.assertEqual(matrix.row(2), [3, 4])
 def test_extract_row_where_numbers_have_different_widths(self):
 matrix = Matrix("1 2\n10 20")
 self.assertEqual(matrix.row(2), [10, 20])
 def test_can_extract_row_from_non_square_matrix_with_no_corresponding_column(self):
 matrix = Matrix("1 2 3\n4 5 6\n7 8 9\n8 7 6")
 self.assertEqual(matrix.row(4), [8, 7, 6])
 def test_extract_column_from_one_number_matrix(self):
 matrix = Matrix("1")
 self.assertEqual(matrix.column(1), [1])
 def test_can_extract_column(self):
 matrix = Matrix("1 2 3\n4 5 6\n7 8 9")
 self.assertEqual(matrix.column(3), [3, 6, 9])
 def test_can_extract_column_from_non_square_matrix_with_no_corresponding_row(self):
 matrix = Matrix("1 2 3 4\n5 6 7 8\n9 8 7 6")
 self.assertEqual(matrix.column(4), [4, 8, 6])
 def test_extract_column_where_numbers_have_different_widths(self):
 matrix = Matrix("89 1903 3\n18 3 1\n9 4 800")
 self.assertEqual(matrix.column(2), [1903, 3, 4])
if __name__ == "__main__":
 unittest.main()
```

Answer 1

I would make a few suggestions, rather than just saving the input string it would be better, as you suggest, to save the output of the get_rows function in memory, so you don't have to recreate it every time. Similarly when you want an individual row or column, it seems inefficient to gather all the rows, (and then all the columns) just to reject all but one of them. Your comments say produces i-th (column/row)... but the variable is referred to as index

Minor points: You don't need to save rows as a variable in _get_columns, you can just use [list(row) for row in zip(*self._get_rows())]

you could reduce _get_rows to one line : [[int(char) for char in row_string.split()] for row_string in self.matrix_string.split('\n')]

I'm assuming that you don't need to check if the input is invalid, because that isn't part of the testing suite. However you may want to do it anyway if you plan on using this class again.

In terms of style the code is good. The first comment under class Matrix doesn't really say anything and there are more newlines than are necessary, but other than that everything looks good.

You could try something like:

class Matrix:
 """Class representation of an integer matrix indexed from 1
 ...
 Attributes
 ----------
 rows: List[List(int)]
 A list of the rows of values in the matrix.
 Methods
 -------
 row(self, i)
 returns the i-th row.
 column(self, i)
 returns the i-th column.
 """
 def __init__(self, matrix_string):
 """Save a list of rows from the input string.
 - matrix_string: A space and new line seperated string of values
 in the matrix.
 """
 self.rows = self._get_rows(matrix_string)
 def _get_rows(self, matrix_string):
 """Produce a list of rows of the matrix.
 - matrix_string: A space and new line seperated string of values
 in the matrix.
 """
 rows = []
 for row_string in matrix_string.split('\n'):
 rows.append([int(char) for char in row_string.split()])
 return rows
 def _get_columns(self):
 """Produce a list of columns of the matrix."""
 return [list(row) for row in zip(*self.rows)]
 def row(self, i):
 """Produce i-th row of the matrix with first index being 1."""
 return self.rows[i - 1]
 def column(self, i):
 """Produce i-th column of the matrix with first index being 1."""
 return [row[i - 1] for row in self.rows]

or you could drop the _get_rows and change the __init__ to:

 def __init__(self, matrix_string):
 """Save a list of rows from the input string.
 - matrix_string: A space and new line seperated string of values
 in the matrix.
 """
 self.rows = [[int(x) for x in row.split()] for row in matrix_string.split("\n")]

In terms of performance:

using cProfile and the following script:

if __name__ == "__main__":
 for _ in range(100000):
 matrix = Matrix("1 2 3 4\n5 6 7 8\n9 8 7 6\n1000 2000 3000 4000")
 for i in range(1, 5):
 matrix.column(i)
 matrix.row(i)

Produces for the old matrix definition: 12900228 function calls (12900222 primitive calls) in 10.239 seconds For the my definition: 1900228 function calls (1900222 primitive calls) in 1.359 seconds

• Konstantin Kostanzhoglo: Thanks for your suggestions! I'm aware that _get_rows() and _get_columns() can be one-liners, though I didn't want to make too long list comprehensions. I'm wondering what's wrong with my newlines, shouldn't we put a new line after a docstring?
• MindOfMetalAndWheels: It's up to your sense of style, I would say that too many new lines spread the code out unnecessarily. Just do what ever is clearest to your eye or what matches the other code in your code base.
• MindOfMetalAndWheels: In terms of performance:
• Konstantin Kostanzhoglo: Yeah, that's clear that my version does redundant computations.
• Konstantin Kostanzhoglo: Have you any ideas how to define _get_columns() via list comprehensions or similar elegant mechanism without using _get_rows_()?