Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Behavior of mmulStrassen #114

Open
holub008 opened this issue Dec 20, 2020 · 2 comments
Open

Behavior of mmulStrassen #114

holub008 opened this issue Dec 20, 2020 · 2 comments

Comments

@holub008
Copy link
Contributor

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
Copy link
Contributor Author

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

@targos
Copy link
Member

targos commented Dec 22, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants