[SYSTEMDS-3948] Row-wise Sparsity Estimator

[ywcb00]

Hi,
this PR adds the row-wise sparsity estimator from:
Lin, Chunxu, Wensheng Luo, Yixiang Fang, Chenhao Ma, Xilin Liu and Yuchi Ma;
On Efficient Large Sparse Matrix Chain Multiplication;
Proceedings of the ACM on Management of Data 2 (2024): 1 - 27.

Note that the row sparsity propagation, as described in the publication, applies to MM chains only. Other operations use fallback methods for sparsity estimation w/ row sparsity vectors, which create a cut in the sparsity estimation DAG.

[Baunsgaard]

Some comments. hope it helps.

[Baunsgaard]

You can optionally have all of these classes extend another class where you add these tests. Instead of copying the code on all of them.

[ywcb00]

Thank you for the comment. I do not fully understand how you would consolidate them. Could you please clarify?

[Baunsgaard]

What I ment was something like :

OpElemWTest extends OpElemBaseTest {

}

and then you define a new OpElemBaseTest class that contains the variables all the different OpElem test cases share, and define in that base test the fundamental tests that all of the OpElem should run.

I hope this would work nicely, since it makes it easy to make the same test flavour run in all of the variations.

[ywcb00]

Thank you again for the explanation, I gave it another thought. While I also like such a hierarchical structure for tests in general, I cannot see the clear advantage here--given that I understood your suggestion correctly.
There are five common variables among the test cases m, n, sparsity, mult, plus, which would then be defined inside OPElemBaseTest class instead of OPElemWTest class. I would not consolidate tests for multiplication and addition (neither for different estimators) in one single function, as this would break the typical structure in which tests are defined (right?). Hence, I think such a base class would just add another class file, or have I misunderstood something?

[Baunsgaard]

I still think the restructure would be advantageous. Since many of these tests have the same fundamental structure, and are not very well tested with only one matrix size. Alternatively you could change the entire class to be a parameterized test instead. That would also help you make more tests easily.

e.g.

@RunWith(value = Parameterized.class)
public class OpElm {
  public OpElm(Estimator e, int m, int n, double s, BinaryOp BOp){
   ...
  }


  @Parameters
  public static Collection<Object[]> data() {
  ...


  }


}

See for instance src/test/java/org/apache/sysds/test/component/compress/CompressedTestBase.java.

To make the tests faster as well you can set as input the actual matrix, and reuse the same matrix across many tests. The current structure create the matrix for each test, and that is by far the most expensive part of these tests.

[ywcb00]

Thank you for your patience. I really like the suggestion of parameterized tests, and changed it accordingly. :)

I also tried to reuse the input matrices instead of generating them from scratch every time. However, as it did not result in a speedup of the tests, I removed it again.

[codecov]

Codecov Report

❌ Patch coverage is 76.28205% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.34%. Comparing base (c2d0e5a) to head (4855b08).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
.../org/apache/sysds/hops/estim/EstimatorRowWise.java	76.28%	24 Missing and 13 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2466      +/-   ##
============================================
+ Coverage     71.29%   71.34%   +0.04%     
- Complexity    48574    48686     +112     
============================================
  Files          1567     1569       +2     
  Lines        188401   188776     +375     
  Branches      36980    37040      +60     
============================================
+ Hits         134325   134673     +348     
+ Misses        43630    43628       -2     
- Partials      10446    10475      +29     

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

[ywcb00]

Thank you for the comments @Baunsgaard . I have updated the PR accordingly. :)

[Baunsgaard]

A few comments

[ywcb00]

Thank you for the valuable comments. :)

[Baunsgaard]

PR LGTM, there are only micro optimizations to do now.

[Baunsgaard]

this is a wrong statement.

The estimator here is the uniform estimator ( also called average-case or in fancy terms Naive Bayes estimator )

It is not the 'best', if that even exists, and it is an interesting point to consider if you are going in this direction for your research.

a clear way to see it is not best or worst case is:

rsM1 = [1,0,0] and rsM2 = [1/3,1/3,1/3]

then:

rsOut[0] = 1 - (1 - 1·1/3)^3
         = 1 - (2/3)^3
         = 1 - 8/27
         = 19/27 ≈ 0.704

which is impossible to get in practice.

for these values the matrices could be :

M1 = [1 1 1]      M2 = [1 0 0]
     [0 0 0]           [0 1 0]
     [0 0 0]           [0 0 1]

C = M1·M2 = [1 1 1]   →  row-0 actual density = 1.0
            [0 0 0]
            [0 0 0]

formular says 0.704 while the matrix actually is 1.0

another fun one is :

M1 = [1 1 1]      M2 = [1 0 0]
     [0 0 0]           [1 0 0]
     [0 0 0]           [1 0 0]

C = M1·M2 = [3 0 0]   →  row-0 actual density = 1/3 ≈ 0.333
            [0 0 0]
            [0 0 0]

it ends up as 0.333 density, but the estimator says 0.7 again.

[Baunsgaard]

consider on code like this to do :

currentVal *= 1.0 - (rsM1[i] * rsM2[j]);

you do not need to write the cast.

[Baunsgaard]

also consider early out, if you encounter zero, do not enter the inner loop

rsM1[i] == 0;

[Baunsgaard]

similar to the cbind this depends on number of rows/cols in the input, right ?