[SYSTEMDS-3729] Add roll reorg operations in CP, python script test

[min-guk]

This PR implements a new roll reorg function in CP and provides test scripts using Python.

1. Implementation of roll function for dense and sparse matrices in CP, along with test code

• Similar to rev, dense column vectors are rolled column-wise, while the rest are rolled row-wise.

2. Support for Python test code and additional test cases

• Since R does not provide a roll function, baseline test scripts were written using Python's numpy roll function.
• Added support for executing the test code in Python. Although there is some overlap with the R script execution code, I avoided modifying or refactoring the existing code to minimize changes.
• However, due to slight differences in the mtx format between R and Python, I made the following modifications to the mtx file input/output code:
  • TestUtils::writeTestMatrix(): The last number in the mtx header should represent the number of non-zero elements in the matrix, but the previous input test matrix was showing the total number of elements, which caused an error in Python. (https://networkrepository.com/mtx-matrix-market-format.html)
  • TestUtils::readRMatrixFromFS(): While R writes the mtx header in 2 lines, Python's scipy writes it in 3 lines, including an extra line with just a %. I modified the code to ignore this blank line.

Please feel free to share any feedback or suggestions regarding the implementation direction.

[Baunsgaard]

Looks very good, just missing a few things.

[min-guk]

Hello Sebastian,
Thank you for the detailed feedback. I've addressed everything except for runPythonScript(), for which I left a comment above and would appreciate your input.

Additionally, the rollTest Python script works fine locally, but there are environment issues on GitHub Actions. I'll resolve this soon by updating the javaTests.yml.

[min-guk]

The Java tests pass locally but continue to fail in GitHub Actions because the numpy package was not installed in the test Docker image.

I have updated the testsysds.Dockerfile, so I would appreciate it if you could execute the Docker Test Image Update in GitHub Actions after the Pull Request is merged.

[Baunsgaard]

It is a bit unfortunate that the python is not installed in our test image (see specific comment for more details). Therefore, it is hard for you to call python and test the way you want to.

Alternatively, I can suggest and would actually recommend:

1. Write the test up in java, where you know what the result should be without comparing to Numpy.
2. And then in our Python API, write the test again, to compare to Numpy. (we have many examples of doing this in recent commits)

[min-guk]

I have created a Python API and added Python test code for roll function.

I have rolled back the code related to the MTX file reader/writer, which was separated into another PR, as well as the previously modified Java test code and Docker file script.

[codecov]

Codecov Report

Attention: Patch coverage is 16.07143% with 94 lines in your changes missing coverage. Please review.

Project coverage is 70.58%. Comparing base (7daa590) to head (5c23bbe).
Report is 23 commits behind head on main.

Files with missing lines	Patch %	Lines
...ache/sysds/runtime/matrix/data/LibMatrixReorg.java	10.90%	48 Missing and 1 partial ⚠️
...ds/runtime/instructions/cp/ReorgCPInstruction.java	12.50%	12 Missing and 2 partials ⚠️
src/main/java/org/apache/sysds/hops/ReorgOp.java	9.09%	10 Missing ⚠️
...apache/sysds/parser/BuiltinFunctionExpression.java	0.00%	8 Missing ⚠️
...in/java/org/apache/sysds/parser/DMLTranslator.java	0.00%	6 Missing ⚠️
...pache/sysds/runtime/functionobjects/RollIndex.java	60.00%	4 Missing ⚠️
src/main/java/org/apache/sysds/lops/Transform.java	0.00%	2 Missing ⚠️
.../apache/sysds/runtime/matrix/data/MatrixBlock.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2103      +/-   ##
============================================
+ Coverage     70.50%   70.58%   +0.08%     
- Complexity    42071    42214     +143     
============================================
  Files          1441     1442       +1     
  Lines        162297   162526     +229     
  Branches      31626    31672      +46     
============================================
+ Hits         114425   114719     +294     
+ Misses        38832    38817      -15     
+ Partials       9040     8990      -50     

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

[min-guk]

Previously, the roll function incorrectly handled sparse matrix indices, leading to a test failure, but I have resolved the issue.

[Baunsgaard]

LGTM the implementation looks solid.
However, we are missing code coverage in the test. The python test is great to ensure that we give the same results as numpy, however out code coverage only include internal java tests.

I would suggest to add a component test that just covers the call to LibMatrixReorg,
where you test sparse inputs and dense inputs.
It can be inspired by: src/test/java/org/apache/sysds/test/component/matrix/binary/BinaryOpTest.java

[min-guk]

Thank you for continuously providing valuable feedback.

As I'm encountering code coverage for the first time, I have some questions. I've summarized the current situation and requests below as I understand them.

Could you please confirm if this is correct?

• Situation Summary:

  • Java Test Rejection:
    • Attempted to compare the Java test of the roll function with NumPy, but it was rejected because we cannot install Python packages in the Docker image.
  • Limitations of the Python Test:
    • Verified correct implementation on dense and sparse matrices through Python tests comparing with NumPy.
    • However, since it uses the statically built Python API, it is not included in code coverage via Codecov.
  • Code Coverage Issue:
    • To improve code coverage, a Java-based test is necessary.

• Request:

  • Add Java Test:
    • Need to write a Java test for the roll function without using NumPy.
    • The test should generate sparse and dense matrices, apply the roll function to each, and verify that the results are identical.
    • The test code should be located at src/test/java/org/apache/sysds/test/component/matrix/libMatrixReorg/RollTest.java (or src/test/java/org/apache/sysds/test/functions/reorg/FullRollTest.java).

[Baunsgaard]

Thank you for continuously providing valuable feedback.

As I'm encountering code coverage for the first time, I have some questions. I've summarized the current situation and requests below as I understand them.

Could you please confirm if this is correct?

* Situation Summary:
  
  * Java Test Rejection:
    
    * Attempted to compare the Java test of the roll function with NumPy, but it was rejected because we cannot install Python packages in the Docker image.
  * Limitations of the Python Test:
    
    * Verified correct implementation on dense and sparse matrices through Python tests comparing with NumPy.
    * However, since it uses the statically built Python API, it is not included in code coverage via Codecov.
  * Code Coverage Issue:
    
    * To improve code coverage, a Java-based test is necessary.

* Request:
  
  * Add Java Test:
    
    * Need to write a Java test for the roll function without using NumPy.
    * The test should generate sparse and dense matrices, apply the roll function to each, and verify that the results are identical.
    * The test code should be located at `src/test/java/org/apache/sysds/test/component/matrix/libMatrixReorg/RollTest.java` (or `src/test/java/org/apache/sysds/test/functions/reorg/FullRollTest.java`).

Yes this sounds correct.

Therefore:

1. keep the python tests that you made, because they ensure compatibility with numpy,
2. make internal tests (component tests) that, as you say, verify the roll function without using numpy and verify the results correctly.

Thanks.

[min-guk]

I have added the JavaTest as mentioned earlier. I would appreciate it if you could review it. Thank you!

[min-guk]

I fixed an issue where the roll function was not properly invoked due to an incorrect output file setting and the lack of calling recomputeNonZeros in the Java test.

[mboehm7]

LGTM - @min-guk could you please rebase this PR (there are quite a number of merge conflicts over these many commits) and then I would merge it in.

[min-guk]

Thank you @mboehm7. I have proceeded with the rebase as requested.

Once the PR is merged, I will immediately create a PR for the Spark version as well.

Additionally, I have been studying how to implement it using FED, but I’m finding it a bit difficult to grasp. Could you perhaps provide a brief explanation or suggest some reference materials I could look into?

[mboehm7]

OK, this was not a git --pull rebase orign main but a merge, which meant I had to rebase and resolve these conflicts myself (but that's fine). Thanks for the contribution @min-guk.

Here are some papers for the federated backend:
https://mboehm7.github.io/resources/sigmod2021c_exdra.pdf
https://mboehm7.github.io/resources/cikm2022.pdf

[min-guk]

@mboehm7 I apologize for the inconvenience.

It seems I made a mistake due to my inexperience with Git and GitHub. I will make sure to study more to prevent this from happening again in the future.

Also, thank you for recommending the papers. I will take the time to read and study them.