Matrix multiplication gives wrong output and throws error for compatible matrices

[Maxouwe]

It seems that sometimes the output of Matrix.mul is incorrect. For example:
a = [[1, 3, -5], [-6, 3, -2]]; b = [[1, 3, 4, 6], [1, 2, 3, 5], [1, 2, 34, 2]]; Matrix.mul(a, b)
Which gives the output [ [ -1, -1 ], [ -5, -16 ] ]
and which should be [ [ -1, -1, -157, 11 ], [ -5, -16, -83, -25 ] ]
I also get an error for certain multiplications eventhough the matrices are elligible for multiplication.
For example:
a = [[1, 1, 1], [1, 1, 1], [1, 1, 1]]; b = [[1, 1], [1, 1], [1, 1]]; Matrix.mul(a, b)
This should have an output of [ [ 3, 3 ], [ 3, 3 ], [ 3, 3 ] ]
But instead this is shown in the post window:
`ERROR: binary operator '' failed.
RECEIVER:
nil
ARGS:
Instance of Array { (00000219D9F9C378, gc=F0, fmt=01, flg=00, set=02)
indexed slots [3]
0 : Integer 1
1 : Integer 1
2 : Integer 1
}
nil
CALL STACK:
DoesNotUnderstandError:reportError
arg this =
Nil:handleError
arg this = nil
arg error =
Thread:handleError
arg this =
arg error =
Object:throw
arg this =
Object:performBinaryOpOnSomething
arg this = nil
arg aSelector = ''
arg thing = [*3]
arg adverb = nil
< FunctionDef in Method Meta_Matrix:mul >
arg u = 2
Integer:do
arg this = 3
arg function =
var i = 2
ArrayedCollection:do
arg this = [*3]
arg function =
var i = 0
Meta_Matrix:mul
arg this =
arg m1 = [*3]
arg m2 = [*3]
var m2s = [*2]
var out = [3]
var size = 3
Interpreter:interpretPrintCmdLine
arg this =
var res = nil
var func =
var code = "Matrix.mul(a, b)"
var doc = nil
var ideClass =
Process:interpretPrintCmdLine
arg this =
^^ ERROR: binary operator '' failed.
RECEIVER: nil

`

[joslloand]

Not bothering to do any thinking (that's what the interpreter is for!):

// example from help for Matrix: *mul
a = [ [ 1, 2 ],[ 3, 4 ] ];
b = [ [ 1, 2 ],[ 1, 2 ] ];
b = Array.series(4, 5, 2).clump(2);  // test

~result = Matrix.mul(a, b);
~result.class;  // this doesn't return a matrix! - as a class method, I would expect it would!!!!

// try with matrices
~result = Matrix.mul(Matrix.with(a), Matrix.with(b));  // ERROR: Primitive '_BasicAt' failed.


// multiply matrices via Matrix: -mulMatrix
~result = Matrix.with(a).mulMatrix(Matrix.with(b));  // correct!
~result.class;  // this does... as expected...


// @Maxouwe example #1
a = [ [ 1, 3, -5 ], [ -6, 3, -2 ] ];
b = [ [ 1, 3, 4, 6 ], [ 1, 2, 3, 5 ], [ 1, 2, 34, 2 ] ];
~result = Matrix.mul(a, b);  // wrong answer!

// reframe...
~result = Matrix.with(a).mulMatrix(Matrix.with(b));  // right answer!

// @Maxouwe example #2
a = [ [ 1, 1, 1 ], [ 1, 1, 1 ], [ 1, 1, 1 ] ];
b = [ [ 1, 1 ], [ 1, 1 ], [ 1, 1 ] ];
~result = Matrix.mul(a, b);  // ERROR: binary operator '*' failed.

// reframe...
~result = Matrix.with(a).mulMatrix(Matrix.with(b));  // right answer!



// try Matrix: -mul
Matrix.with(a).mul(2);  // fine...

Matrix.with(a).mul(b)  // nope...
Matrix.with(a).mul(Matrix.with(b))  // nope...

Matrix.with(b).mul(a)  // nope...
Matrix.with(b).mul(Matrix.with(a))  // nope...


// what about...?
/*
... this works...
*/
Matrix.mul(a, a.flop);
Matrix.mul(a.flop, a);

// compare...
Matrix.with(a).mulMatrix(Matrix.with(a.flop));
Matrix.with(a.flop).mulMatrix(Matrix.with(a));

[joslloand]

Wrong answers should not be expected, but I would say my biggest complaint is that the class method Matrix: *mul returns an Array instance rather than a Matrix.

As far as the incorrect results go, for Matrix, none of the other methods appear to depend upon the *mul or -mul methods, so that's good news.

If this is indeed the case, then the observed issue would only impact users who have built code around the current misbehaviour. It would be a good idea to address.

Likely the most simple solution would be to refactor around the undocumented instance methods -mulMatrix and -mulNumber.

---

BEFORE doing so, however, we'll want to review to see that there aren't any dependencies within MathLib regarding the current behaviour. If there are, some attentive refactoring is to be involved.

If the current performance does have a use case, then that should be noted, and/or promoted (and renamed?) for that specific case... AND, the user should be directed to use -mulMatrix for matrix multiplication.

NOTE: I do feel the class method should return a Matrix instance, otherwise the current implementation looks like an Array class method to me.