Fixed error in matrix multiplication that didn't multiple non-square …#898
Fixed error in matrix multiplication that didn't multiple non-square …#898internetdhruv wants to merge 2 commits intoTheAlgorithms:masterfrom
Conversation
…matrices. Also added checks for invalid dimensions
StephenGemin
left a comment
StephenGemin
left a comment
There was a problem hiding this comment.
Maybe I'm just missing the point of this module, but I think we can do the same thing, with less code.
| @@ -15,12 +15,19 @@ def scalarMultiply(matrix , n): | |||
|
|
|||
| def multiply(matrix_a, matrix_b): | |||
| matrix_c = [] | |||
There was a problem hiding this comment.
with numpy arrays, no need to define matrix_c at the beginning of the function. Also, since I can't access the beginning of this module would need to add "import numpy as np".
Below is a suggestion to reduce the number of code lines. Maybe I'm missing the point of this module, but using numpy would be a lot more efficient. I know the rest of the doc has no docstring, but I figured I would add one in the suggestion.
You can the entire function with just 8 lines of code.
| matrix_c = [] | |
| """ | |
| :param matrix_a: matrix as a list or array, 1st matrix that defines #columns for matrix rule [a1 x a2] | |
| :type matrix_a: list of int or list of float | |
| :param matrix_b: matrix as a list or array, 2nd matrix that defines #rows for matrix rule [b1 x b2] | |
| :type matrix_b: list of int or list of float | |
| :return: numpy array [a1 x b2] | |
| """ | |
| matrix_a = np.array(matrix_a) | |
| matrix_b = np.array(matrix_b) | |
| try: | |
| matrix_c = np.dot(matrix_a, matrix_b) | |
| return matrix_c | |
| except ValueError: | |
| raise |
There was a problem hiding this comment.
This definitely works. In fact, a lot of stuff in this project can be done using in-built/library functions, but I think the idea of this project is to show how the core algorithms work.
Using numpy.dot would definitely make it work in lesser lines of code, and possibly even be faster, but that level of abstraction would hide what exactly is happening.
I may be wrong though, and if what we're going for is smaller code and speed, your suggestion would work (Although I think numpy.matmul would be a better choice)
There was a problem hiding this comment.
That makes sense. You're right on both points ☺. Best leave your initial commit than.
I never heard of numpy.matmul, now I know.
https://www.numpy.org/devdocs/reference/generated/numpy.dot.html
TLDR: If both a and b are 2-D arrays, it is matrix multiplication, but using matmul or a @ b is preferred.
Appreciate the feedback!
Update matrix ops Within the matrix folder: 1. Removed init.py 2. Added pytests for matrix operations as it was difficult to see if things were working correctly just running it through print statements on the main file. There were edge cases that the algorithms did not account for. Made the following changes to matrix_operation.py 1. added matrix subtraction to matrix_operation.py 2. added matrix size checks for addition and subtraction as there were previously no checks 3. fixed typo in matrix multiplication loop that was in Pull Request TheAlgorithms#898 on TheAlgorithms/Python 4. PEP8 changes
2 participants
…matrices. Also added checks for invalid dimensions
The original code for matrix multiplication did not support non square matrices (reference to #837 )
The function also did not check if the 2 matrices can even be multiplied.
The error has been fixed, and the check has been added