matrix icon indicating copy to clipboard operation
matrix copied to clipboard

Behavior of mmulStrassen

Open holub008 opened this issue 4 years ago • 2 comments

mmulStrassen does not compute a matrix multiplication, as its name implies.

new Matrix([[1, 2]]).mmulStrassen(new Matrix([[1,2],[3,4]])).to2DArray()
// returns [[7, 10], [0,0]]

The caller gets back a multiplication result, zero padded to a square matrix. I believe this is done intentionally in embed, see comment here. It's unclear why the returned result wouldn't be trimmed to the correct dimensions (perhaps it made the recursion easier to implement?).

While this may not be unintentional, I think it's misleading at best; it's not documented anywhere I could find.

holub008 avatar Dec 20 '20 21:12 holub008

Also, it looks like we have no tests on mmulStrassen for inputs > 512x512, which means this algo is functionally untested.

holub008 avatar Dec 20 '20 22:12 holub008

mmulStrassen was implemented as an experiment to see if we could optimize the multiplication. It's not well tested and documented because of that.

We should check if any time can still be gained from that implementation. If not, I would suggest to deprecate it in the documentation and keep it as it is (to avoid a breaking change in the API).

targos avatar Dec 22 '20 09:12 targos