Skip to content

[SYSTEMDS-3941] Add new rewrites#2460

Open
smelihportakal wants to merge 1 commit intoapache:mainfrom
smelihportakal:SYSTEMDS-3941
Open

[SYSTEMDS-3941] Add new rewrites#2460
smelihportakal wants to merge 1 commit intoapache:mainfrom
smelihportakal:SYSTEMDS-3941

Conversation

@smelihportakal
Copy link
Copy Markdown

This PR implements the rewrites requested in SYSTEMDS-3941.

Changes:

  • Add a static rewrite for rowSums(a*A) => a*rowSums(A)
  • Add a static rewrite for colSums(a*A) => a*colSums(A)
  • Add a dynamic rewrite for sum(matrix(a, rows=b, cols=c)) => a*b*c
  • Add tests for all new rewrites under src/test/java/org/apache/sysds/test/functions/rewrite
  • Adjust RewriteFusedRandTest to avoid interference from the new rowSums rewrite

Validation:

  • Added rowSums rewrite tests with rewrites enabled and disabled
  • Added colSums rewrite tests with rewrites enabled and disabled
  • Added constant matrix sum rewrite tests with rewrites enabled and disabled
  • Updated RewriteFusedRandTest so it continues to test the intended fused random rewrite behavior

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 23, 2026

Codecov Report

❌ Patch coverage is 92.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.32%. Comparing base (72ce253) to head (f0032ee).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
.../rewrite/RewriteAlgebraicSimplificationStatic.java 87.50% 0 Missing and 4 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2460      +/-   ##
============================================
- Coverage     71.38%   71.32%   -0.06%     
- Complexity    48392    48587     +195     
============================================
  Files          1561     1567       +6     
  Lines        187293   188401    +1108     
  Branches      36762    36980     +218     
============================================
+ Hits         133706   134385     +679     
- Misses        43233    43581     +348     
- Partials      10354    10435      +81     

☔ View full report in Codecov by Sentry.
📢 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.

@janniklinde
Copy link
Copy Markdown
Contributor

Thanks for the additional rewrite implementations and corresponding tests @smelihportakal. In your tests, could you validate that your rewrites are actually applied as an additional assert? You could use the heavy hitters similar to RewriteSimplifyNegatedSubtractionTest (e.g., Assert.assertEquals(1, Statistics.getCPHeavyHitterCount("-"));).

For colsum/rowsum pushdown, you could test an alternative expression such as 2*colSums(3*X), where after your rewrite there should only be a single multiply present as a heavy hitter.

This patch adds two algebraic simplification rewrites. First, it pushes scalar multiplication through rowSums and colSums, rewriting rowSums(a*A) to a*rowSums(A) and colSums(a*A) to a*colSums(A). Second, it simplifies sum(matrix(a, rows=b, cols=c)) to a*b*c when the matrix dimensions are known. The patch also adds rewrite tests for rowSums, colSums, and constant-value matrix sums with rewrites enabled and disabled to validate correctness against the reference outputs.
@smelihportakal
Copy link
Copy Markdown
Author

Thanks for the additional rewrite implementations and corresponding tests @smelihportakal. In your tests, could you validate that your rewrites are actually applied as an additional assert? You could use the heavy hitters similar to RewriteSimplifyNegatedSubtractionTest (e.g., Assert.assertEquals(1, Statistics.getCPHeavyHitterCount("-"));).

For colsum/rowsum pushdown, you could test an alternative expression such as 2*colSums(3*X), where after your rewrite there should only be a single multiply present as a heavy hitter.

Implemented the requested rewrite-validation asserts in the tests.
For SimplifySumConstantMatrix I added a heavy-hitter check to ensure the constant matrix is not materialized when the rewrite is enabled:

Assert.assertFalse(heavyHittersContainsString("rand"));

For the row/col sum pushdown tests, I changed the left-scalar cases to use expressions of the form:

2 * rowSums(3 * X)
2 * colSums(3 * X)

and added heavy-hitter assertions to validate the rewrite effect. In the rewrite-enabled case, there is a single scalar multiply heavy hitter:

Assert.assertEquals(1, Statistics.getCPHeavyHitterCount("n*"));

and in the no-rewrite case, there are two elementwise multiply heavy hitters:

Assert.assertEquals(2, Statistics.getCPHeavyHitterCount("*"));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

2 participants