Skip to content

fix: mmulStrassen returns the matrix product for every input shape - #217

Closed
tayal-sarthak wants to merge 6 commits into
mljs:mainfrom
tayal-sarthak:strassen
Closed

fix: mmulStrassen returns the matrix product for every input shape#217
tayal-sarthak wants to merge 6 commits into
mljs:mainfrom
tayal-sarthak:strassen

Conversation

@tayal-sarthak

Copy link
Copy Markdown

Closes #114.

Summary

mmulStrassen does not compute the matrix product. Three separate defects were found, the middle one silent.

1. The two off diagonal output blocks are swapped

The recombination step writes C12 where C21 belongs and the reverse:

result = result.setSubMatrix(c12, c11.rows, 0);    // top right block, written bottom left
result = result.setSubMatrix(c21, 0, c11.columns); // bottom left block, written top right

The seven Strassen products themselves are correct, only the placement was crossed.

The recursive path activates once both dimensions pass 512, below that the call is handed to mmul, which is why nothing caught this. On main, with small integers that keep every intermediate exact in a float64:

shape entries disagreeing with mmul
512 x 512 0 of 262144
513 x 513 130888 of 263169
600 x 600 179092 of 360000
1024 x 1024 522202 of 1048576

The first disagreement always sits at column n / 2, the start of the top right block. Roughly half the result is wrong, returned with no error.

2. Shapes where max(r1, r2) differs from max(c1, c2) crash

Both operands were padded to max(r1, r2) by max(c1, c2), which is only a valid pair to multiply when those two happen to be equal. A 3x2 by 2x4 product raised TypeError: Cannot read properties of undefined (reading '0') from inside the multiplication.

Padding into a common square of side max(r1, c1, r2, c2) fixes this. Zeros never reach the top left r1 by c2 corner, which is the part that gets returned.

3. The result was zero padded, the case reported in the issue

new Matrix([[1, 2]]).mmulStrassen(new Matrix([[1, 2], [3, 4]])) gave [[7, 10], [0, 0]]. The product now comes back at r1 by c2, matching mmul.

Result

mmulStrassen agrees with mmul on every shape tried, including sizes above the recursion threshold where the two now match entry for entry.

On the performance question raised in the thread, the algorithm does earn its place at the sizes it engages, measured on a 1024 x 1024 product:

time
mmul 1835 ms
mmulStrassen 1080 ms

Notes

A degenerate pair such as 0x2 by 2x0 now comes back at 0x0 rather than padded to 2x2. That shape was previously pinned in place with a comment recording 0x0 as the mathematically correct answer.

The dimension mismatch warning is untouched, since mmul does not validate its operands either.

@codecov

codecov Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.15%. Comparing base (1a4a05a) to head (829a842).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #217      +/-   ##
==========================================
+ Coverage   68.88%   70.15%   +1.26%     
==========================================
  Files          49       49              
  Lines        5904     5910       +6     
  Branches     1060     1076      +16     
==========================================
+ Hits         4067     4146      +79     
+ Misses       1824     1751      -73     
  Partials       13       13              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@targos targos closed this Aug 6, 2026
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

Successfully merging this pull request may close these issues.

Behavior of mmulStrassen

2 participants