Skip to main content
Added a paragraph about module names
Source Link
Aemyl
  • 850
  • 1
  • 8
  • 18

I like your implementation, most of it looks good to me. The issues I see are only minor:

1. blank lines

from PEP 8:

Surround top-level function and class definitions with two blank lines.

Here, I would count the two comment lines above the fromdim definition as part of the definition and insert a second blank line between imports and comments, as well as one more blank line between fromdim and Matrix

2. Matrix.tpose()

I think this method should be named transpose.

3. dunder methods

Your dunder methods do multiple things at once. For the __mul__ that makes sense to me, but I would have expected Matrix(...) + 1 to raise a TypeError. However, it looks like you wanted that feature, so I don't mind too much. You could move the code for the matrix multiplication into another dunder method __matmul__, which would allow you to write matrix1 @ matrix2. In that case, I would keep matrix1 * matrix2 as an option for people who don't know about @ or don't like it.

4. module name

From your testing code I can see you called your module PyMatrix. This follows the old naming convention from Python 2. The current guideline prefers a name like pymatrix or py_matrix.

I like your implementation, most of it looks good to me. The issues I see are only minor:

1. blank lines

from PEP 8:

Surround top-level function and class definitions with two blank lines.

Here, I would count the two comment lines above the fromdim definition as part of the definition and insert a second blank line between imports and comments, as well as one more blank line between fromdim and Matrix

2. Matrix.tpose()

I think this method should be named transpose

3. dunder methods

Your dunder methods do multiple things at once. For the __mul__ that makes sense to me, but I would have expected Matrix(...) + 1 to raise a TypeError. However, it looks like you wanted that feature, so I don't mind too much. You could move the code for the matrix multiplication into another dunder method __matmul__, which would allow you to write matrix1 @ matrix2. In that case, I would keep matrix1 * matrix2 as an option for people who don't know about @ or don't like it.

I like your implementation, most of it looks good to me. The issues I see are only minor:

1. blank lines

from PEP 8:

Surround top-level function and class definitions with two blank lines.

Here, I would count the two comment lines above the fromdim definition as part of the definition and insert a second blank line between imports and comments, as well as one more blank line between fromdim and Matrix

2. Matrix.tpose()

I think this method should be named transpose.

3. dunder methods

Your dunder methods do multiple things at once. For the __mul__ that makes sense to me, but I would have expected Matrix(...) + 1 to raise a TypeError. However, it looks like you wanted that feature, so I don't mind too much. You could move the code for the matrix multiplication into another dunder method __matmul__, which would allow you to write matrix1 @ matrix2. In that case, I would keep matrix1 * matrix2 as an option for people who don't know about @ or don't like it.

4. module name

From your testing code I can see you called your module PyMatrix. This follows the old naming convention from Python 2. The current guideline prefers a name like pymatrix or py_matrix.

Source Link
Aemyl
  • 850
  • 1
  • 8
  • 18

I like your implementation, most of it looks good to me. The issues I see are only minor:

1. blank lines

from PEP 8:

Surround top-level function and class definitions with two blank lines.

Here, I would count the two comment lines above the fromdim definition as part of the definition and insert a second blank line between imports and comments, as well as one more blank line between fromdim and Matrix

2. Matrix.tpose()

I think this method should be named transpose

3. dunder methods

Your dunder methods do multiple things at once. For the __mul__ that makes sense to me, but I would have expected Matrix(...) + 1 to raise a TypeError. However, it looks like you wanted that feature, so I don't mind too much. You could move the code for the matrix multiplication into another dunder method __matmul__, which would allow you to write matrix1 @ matrix2. In that case, I would keep matrix1 * matrix2 as an option for people who don't know about @ or don't like it.