Skip to content

[SYSTEMDS-3951] MM Chain Optimization w/ Basic Average Estimator#2525

Closed
ywcb00 wants to merge 8 commits into
apache:mainfrom
ywcb00:fix/hops/rewrite/sparse
Closed

[SYSTEMDS-3951] MM Chain Optimization w/ Basic Average Estimator#2525
ywcb00 wants to merge 8 commits into
apache:mainfrom
ywcb00:fix/hops/rewrite/sparse

Conversation

@ywcb00

@ywcb00 ywcb00 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Hi,
this PR changes the estimator for sparse mm chain rewrites from MNC to the metadata average estimator. Integration of MNC for these rewrites is missing the information about the underlying matrix histograms.
This PR also fixes the corresponding test, which previously only validated if there was any mm chain optimization.
Thank you and all the best,
David

@ywcb00 ywcb00 changed the title [SYSTEMDS-3951][WIP] MM Chain Optimization w/ Basic Average Estimator [SYSTEMDS-3951] MM Chain Optimization w/ Basic Average Estimator Jul 2, 2026
@ywcb00

ywcb00 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Thank you very much @Baunsgaard. This was exactly what I was searching for and I changed the test validation accordingly. :)

Does anybody know why the builtin.part2 java test resulted in a timeout and is this behavior related to the current PR? Test output states: Our tests should run faster than 1000 sec each

If this is not related to the PR, I would like to merge the changes soon.

All the best,
David

@Baunsgaard

Copy link
Copy Markdown
Contributor

Thank you very much @Baunsgaard. This was exactly what I was searching for and I changed the test validation accordingly. :)

Does anybody know why the builtin.part2 java test resulted in a timeout and is this behavior related to the current PR? Test output states: Our tests should run faster than 1000 sec each

If this is not related to the PR, I would like to merge the changes soon.

All the best, David

  1. You are welcome.

  2. The part2 seems like it got stuck with a threading error again, my hypothesis is Spark was closed by one thread, and another is waiting for response. **It is not related to your changes **. However, do you want to take a look at that suite to check if there is anywhere we close a spark session in an unsafe manner?

@Baunsgaard Baunsgaard left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few Nits, otherwise LGTM

@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.10345% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.61%. Comparing base (d6aabf6) to head (6aa1556).

Files with missing lines Patch % Lines
...rite/RewriteMatrixMultChainOptimizationSparse.java 90.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2525      +/-   ##
============================================
+ Coverage     71.54%   71.61%   +0.06%     
- Complexity    50240    50270      +30     
============================================
  Files          1623     1623              
  Lines        194317   194314       -3     
  Branches      37965    37965              
============================================
+ Hits         139032   139149     +117     
+ Misses        44366    44264     -102     
+ Partials      10919    10901      -18     

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

@ywcb00
ywcb00 force-pushed the fix/hops/rewrite/sparse branch 2 times, most recently from 840e84d to 76d9d52 Compare July 10, 2026 07:46
@ywcb00
ywcb00 force-pushed the fix/hops/rewrite/sparse branch from 76d9d52 to bad0d56 Compare July 23, 2026 07:31
ywcb00 added 8 commits July 23, 2026 14:02
…): delete content of method to get the input matrices since it does not work and replace it with a not implemented exception for now

chore(main/hops/rewrite/ProgramRewriteStatus.java): remove unused methods and member variables to make clear that the program rewrite status does not hold a variable map
… leaf with its metadata only

	change predicate method to check whether it is a leaf accordingly
… check if the mm chain was rewritten with respect to sparsity by checking for the respective keyword in the trace log

chore(test/AutomatedTestBase.java): add check if buffer is null when checking if a buffer contains a certain string
…stimator by the basic average estimator for matrix multiplication chain rewrites

	NOTE: Missing information about the matrix histograms of input data caused certain limitations for the implementation with the mnc estimator.
… capture and validate the log output to remove the necessity of having a console print in the code
…tighter checks for correct rewrites based on sparsity estimates
…apply proper codestyle

refactor(test/functions/rewrite/RewriteMatrixMultChainOptSparseTest.java): apply proper codestyle
@ywcb00
ywcb00 force-pushed the fix/hops/rewrite/sparse branch from ada0809 to 6aa1556 Compare July 23, 2026 12:02
@ywcb00 ywcb00 closed this Jul 23, 2026
@github-project-automation github-project-automation Bot moved this from In Progress to Done in SystemDS PR Queue Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants