Skip to main content
added 218 characters in body
Source Link
J_H
  • 42k
  • 3
  • 38
  • 157

Please have the """docstring""" clarify that thiswhether you were shooting for is thethe dot product operation, and not a @ matrix multiply operation, or something non-standard. The test suite should include non-square inputs to exercise it. OTOH if this is only defined for the square case, drop one argument or enforce equality with raise or assert.

Please have the """docstring""" clarify that this is the dot product operation, and not a @ matrix multiply operation.

Please have the """docstring""" clarify whether you were shooting for the dot product operation, a @ matrix multiply operation, or something non-standard. The test suite should include non-square inputs to exercise it. OTOH if this is only defined for the square case, drop one argument or enforce equality with raise or assert.

Source Link
J_H
  • 42k
  • 3
  • 38
  • 157

pre-condition / class invariant

class Matrix:
    
    def __init__ ...
        self.arrays = ...
        self.width = len(arrays[0])
        self.height = len(arrays)

This is nice, it's very sensible.

Consider iterating over the rows, to verify that each has the identical self.width length. Think of it as "keeping the caller honest". If we were called in error, at least it will be a "shallow" bug, quickly diagnosed.

comments are for the "why", not the "how"

Please elide these comments.

    # Public member functions
    ...
    # Dunder methods

Names such as cols and transpose (you regrettably called it tpose) explain "hello! I am a public name." A leading _ underscore introduces the name of a _private() helper.

When the code has already spoken eloquently, avoid restating the obvious in a # comment.

an int is a float

Well, not exactly. But kind of.

    def cols(self) -> list[list[int | float]]:

Thank you for the very nice annotation, and the docstring. We can simplify it slightly.

    def cols(self) -> list[list[float]]:

The documentation explains:

when an argument is annotated as having type float, an argument of type int is acceptable

This, despite the fact that e.g. isinstance(3, float) is False. It's a minor exception in how we annotate functions, which proves quite convenient.

Similarly for the various other method signatures.

doctest

    def tpose(self) -> None:
        """
        Transposes the current Matrix, modifying the underlying arrays object. This
        converts a MxN array into an NxM array.

        >>> Matrix([[1, 2, 3], [4, 5, 6]])
        [
            [1, 4]
            [2, 5]
            [3, 6]
        ]
        """

Thank you for supplying a doctest docstring. Usually I find them much more believable than ordinary free-form docstrings, because I am confident that the author automatically validated them. But this one? I don't understand its meaning. It seems to contradict the code.

$ python -m doctest matrix.py
**********************************************************************
File "matrix.py", line 33, in matrix.Matrix.tpose
Failed example:
    Matrix([[1, 2, 3], [4, 5, 6]])
Expected:
    [
        [1, 4]
        [2, 5]
        [3, 6]
    ]
Got:
    [
      [1, 2, 3]
      [4, 5, 6]
    ]
**********************************************************************
1 items had failures:
   1 of   1 in matrix.Matrix.tpose
***Test Failed*** 1 failures.

And please call it transpose().

Also, mutating the underlying representation is kind of bizarre. Yes, the docstring accurately tells us what's going on. But for folks accustomed to \$A'\$ notation, or a.T, this is still going to be somewhat counter-intuitive behavior. The one saving grace is we have a -> None: result annotation which helpfully explains that this can only be evaluated for side effects. So a caller who mistakenly attempts a functional approach will soon be told by mypy that they're doing the wrong thing. Good.

implicit None returned

In __add__ there are three cases: {scalar, conforming matrix, other}. In the "other" case, we implicitly return None. Please don't do that.

  • The signature's annotation promised we would return a Matrix rather than None.
  • Explicitly return None if that's what you intend.

It would be much better to raise if caller offered a bad addend.

Also, how did you expect me to pronounce rdx? After casting about, maybe it is a "right index"? Or a "reflected index"? Please don't use such an abbreviation without a # comment to explain it. Prefer boring conventional index names like i or j.

[Later code suggests that maybe we should pronounce it "row index"?]

commutativity

    def __add__(self, other ...

This is lovely, and m + 0 is the identity function, great.

But what of 0 + m? Please def __radd__() so we don't get "TypeError: unsupported operand type(s) for +: 'int' and 'Matrix'".

matrix multiply

        if isinstance(other, Matrix) and ((self.width, self.height) == (other.width, other.height)):

Please have the """docstring""" clarify that this is the dot product operation, and not a @ matrix multiply operation.

Also, it seems odd that no @ matrix multiply operation is provided.

zeros

def fromdim(width: int, height: int):

The name is odd. Please call it the familiar matlab / numpy zeros(), or make it a private _fromdim().

dimensions

Clearly this works:

    def __getitem__(self, idx: int) -> list:
        return self.arrays[idx]

But it seems like a missed opportunity. In addition to supporting e.g. m[1][2], consider supporting m[(1, 2)].

(Also, please have the signature explain that we return list[float].)

test suite

This is nice, as far as it goes.

print("M + N:", m + n)

It is much better than nothing. But it does not "know the right answer", so it cannot display a Green bar in automated fashion. Rather than require a human to eyeball the result and judge whether it is correct, please encode that in the test code, perhaps with a call to self.assertEqual().