[SYSTEMDS-3941] Add new rewrites#2460
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2460 +/- ##
============================================
- Coverage 71.38% 71.31% -0.08%
- Complexity 48392 48581 +189
============================================
Files 1561 1567 +6
Lines 187293 188401 +1108
Branches 36762 36980 +218
============================================
+ Hits 133706 134357 +651
- Misses 43233 43605 +372
- Partials 10354 10439 +85 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
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 For colsum/rowsum pushdown, you could test an alternative expression such as |
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.
f0032ee to
b32930d
Compare
Implemented the requested rewrite-validation asserts in the tests. For the row/col sum pushdown tests, I changed the left-scalar cases to use expressions of the form: and added heavy-hitter assertions to validate the rewrite effect. In the rewrite-enabled case, there is a single scalar multiply heavy hitter: and in the no-rewrite case, there are two elementwise multiply heavy hitters: |
This PR implements the rewrites requested in SYSTEMDS-3941.
Changes:
rowSums(a*A) => a*rowSums(A)colSums(a*A) => a*colSums(A)sum(matrix(a, rows=b, cols=c)) => a*b*csrc/test/java/org/apache/sysds/test/functions/rewriteRewriteFusedRandTestto avoid interference from the newrowSumsrewriteValidation:
RewriteFusedRandTestso it continues to test the intended fused random rewrite behavior