search

mljs / matrix

Matrix manipulation and computation library
https://mljs.github.io/matrix/
MIT License
358 stars 54 forks source link

Behavior of mmulStrassen #114

Open holub008 opened 3 years ago

holub008 commented 3 years ago

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 commented 3 years ago

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

targos commented 3 years ago

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).