Skip to content

Project: S3779 LZW ColGroup | ASML - #2560

Open
MasterBrain2000 wants to merge 33 commits into
apache:mainfrom
MasterBrain2000:main
Open

Project: S3779 LZW ColGroup | ASML#2560
MasterBrain2000 wants to merge 33 commits into
apache:mainfrom
MasterBrain2000:main

Conversation

@MasterBrain2000

Copy link
Copy Markdown

Group Project for the AMLS Module

@janniklinde

Copy link
Copy Markdown
Contributor

Your code does not build (see error logs). Could you please fix those issues to have a prototype that compiles?

@janniklinde

Copy link
Copy Markdown
Contributor

@MasterBrain2000 @m-ollka @Mancer1 could you please address the issues causing the tests to fail? Thanks

@Mancer1

Mancer1 commented Aug 3, 2026

Copy link
Copy Markdown

@janniklinde, all the build problems have been addressed.

@codecov

codecov Bot commented Aug 3, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.80100% with 82 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.51%. Comparing base (d5f6950) to head (5d0b8b4).
⚠️ Report is 27 commits behind head on main.

Files with missing lines Patch % Lines
...ss/colgroup/ColGroupPiecewiseLinearCompressed.java 89.32% 34 Missing and 39 partials ⚠️
...ress/colgroup/functional/PiecewiseLinearUtils.java 92.22% 3 Missing and 4 partials ⚠️
...he/sysds/runtime/compress/colgroup/ColGroupIO.java 60.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2560      +/-   ##
============================================
- Coverage     71.61%   71.51%   -0.11%     
- Complexity    50132    50658     +526     
============================================
  Files          1614     1631      +17     
  Lines        193986   196282    +2296     
  Branches      37935    38267     +332     
============================================
+ Hits         138925   140373    +1448     
- Misses        44155    44910     +755     
- Partials      10906    10999      +93     

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

@Mancer1

Mancer1 commented Aug 3, 2026

Copy link
Copy Markdown

@janniklinde, for code coverage, aren't tests considered part of it? We have some helper functions from the old DP algorithm, and we used to test them against the new algorithm. These DP helper functions are not used anywhere other than the tests we wrote.

@Mancer1

Mancer1 commented Aug 3, 2026

Copy link
Copy Markdown

@janniklinde, How much code coverage is accepted? We have some null functions that are required to be implemented but never called.

@janniklinde

Copy link
Copy Markdown
Contributor

Generally, all functions that are expected to provide a working implementation should be tested, @Mancer1. Coverage also includes nested function calls, so you should be able to achieve high coverage by testing the publicly available functions of the compressed column group. Please use the tests for existing AColGroup implementations as a reference.

Also, please avoid reimplementing helper functionality that is already available in the test utilities, such as random matrix generation.

Mancer1 and others added 7 commits August 3, 2026 16:43
# Conflicts:
#	src/main/java/org/apache/sysds/runtime/compress/colgroup/ColGroupPiecewiseLinearCompressed.java
#	src/test/java/org/apache/sysds/test/component/compress/colgroup/ColGroupPiecewiseLinearCompressedOperationsTest.java
@Mancer1

Mancer1 commented Aug 7, 2026

Copy link
Copy Markdown

@janniklinde, we have improved the coverage to 95%. I hope that suffices

@Mancer1

Mancer1 commented Aug 8, 2026

Copy link
Copy Markdown

@janniklinde, we have already reformatted our files that we edited. The files that java Codestyle check is showing to reformat are those not part of our PR.

rice@Arnor:~/MasterBrain2000-Project-S3779-LZW-ColGroup$  dev/format-changed.sh
All PR-edited Java lines are correctly formatted.
rice@Arnor:~/MasterBrain2000-Project-S3779-LZW-ColGroup$ git status
On branch main
Your branch is up to date with 'origin/main'.

So, should we leave it be, since we were working on a stale version of main?

@janniklinde

Copy link
Copy Markdown
Contributor

You can simply run the following @Mancer1:

git fetch upstream
dev/format-changed.sh upstream/main

@Mancer1

Mancer1 commented Aug 10, 2026

Copy link
Copy Markdown

You can simply run the following @Mancer1:

git fetch upstream
dev/format-changed.sh upstream/main

@janniklinde, I believe I tried it before. It didn't affect anything.

rice@Arnor:~/MasterBrain2000-Project-S3779-LZW-ColGroup$ git fetch upstream
dev/format-changed.sh upstream/main
remote: Enumerating objects: 299, done.
remote: Counting objects: 100% (172/172), done.
remote: Compressing objects: 100% (90/90), done.
remote: Total 299 (delta 105), reused 82 (delta 82), pack-reused 127 (from 2)
Receiving objects: 100% (299/299), 262.07 KiB | 507.00 KiB/s, done.
Resolving deltas: 100% (122/122), completed with 16 local objects.
From https://github.com/apache/systemds
 * [new branch]            dependabot/github_actions/docker/login-action-4.6.0 -> upstream/dependabot/github_actions/docker/login-action-4.6.0
   7a0e5ca0ac..9dccbc3e60  main       -> upstream/main
All PR-edited Java lines are correctly formatted.
rice@Arnor:~/MasterBrain2000-Project-S3779-LZW-ColGroup$ git status
On branch main
Your branch is up to date with 'origin/main'.
```

@janniklinde
janniklinde self-requested a review August 10, 2026 11:10

@janniklinde janniklinde 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.

Thanks for the updates. I left comments in the code which should still be fixed. Most of them are minor and easy to patch.

Please address those remaining issues to pass the project.

Comment on lines +30 to +34
/**
* Utility methods for piecewise linear compression of matric columns supports compression used the segmented least
* squares algorithm which is implemented with dynamic programming and a successive method, which puts all values in
* a segment till the target loss is exceeded
*/

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.

Comment not up to date, either remove or update and put above class definition

Comment on lines +72 to +92
public static SegmentedRegression compressSuccessivePiecewiseLinear(double[] column, CompressionSettings cs) {
// compute Breakpoints for a Column with a sukzessive breakpoints algorithm

final List<Integer> breakpointsList = computeBreakpointSuccessive(column, cs);
final int[] breakpoints = breakpointsList.stream().mapToInt(Integer::intValue).toArray();

// get values for Regression
final int numSeg = breakpoints.length - 1;
final double[] slopes = new double[numSeg];
final double[] intercepts = new double[numSeg];

// Regress per Segment
for(int seg = 0; seg < numSeg; seg++) {
final int segstart = breakpoints[seg];
final int segEnd = breakpoints[seg + 1];
final double[] line = regressSegment(column, segstart, segEnd);
slopes[seg] = line[0];
intercepts[seg] = line[1];
}
return new SegmentedRegression(breakpoints, slopes, intercepts);
}

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.

These comments are unnecessary. I'd prefer a clear documentation comment for the function itself.

Comment on lines +94 to +117
/**
* computes the segment cost
*
* @param column column values
* @param start start index
* @param end end index
* @return SSE of the regression line over the segment
*/
public static double computeSegmentCost(double[] column, int start, int end) {
final int segSize = end - start;
if(segSize <= 1)
return 0.0;

final double[] ab = regressSegment(column, start, end);
final double slope = ab[0];
final double intercept = ab[1];

double sse = 0.0;
for(int i = start; i < end; i++) {
double err = column[i] - (slope * i + intercept);
sse += err * err;
}
return sse;
}

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.

Can be removed (including the test)

* @param cs compression settings to define the target loss, which should be considered
* @return a piecewise linear compressed column group
*/

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.

Remove line

Comment on lines +1097 to +1119
public static AColGroup compressPiecewiseLinearFunctional(IColIndex colIndexes, MatrixBlock in,
CompressionSettings cs) {

final int numRows = in.getNumRows();
final int numCols = colIndexes.size();
int[][] breakpointsPerCol = new int[numCols][];
double[][] slopesPerCol = new double[numCols][];
double[][] interceptsPerCol = new double[numCols][];

for(int col = 0; col < numCols; col++) {
final int colIdx = colIndexes.get(col);
double[] column = PiecewiseLinearUtils.getColumn(in, colIdx);
PiecewiseLinearUtils.SegmentedRegression fit = PiecewiseLinearUtils
.compressSuccessivePiecewiseLinear(column, cs);
breakpointsPerCol[col] = fit.getBreakpoints();
interceptsPerCol[col] = fit.getIntercepts();
slopesPerCol[col] = fit.getSlopes();

}
return ColGroupPiecewiseLinearCompressed.create(colIndexes, breakpointsPerCol, slopesPerCol, interceptsPerCol,
numRows);

}

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.

Incorrect if cs.transposed = true, ther e you need to iterate over rows

Comment on lines +1326 to +1328
public AColGroup sort() {
return this;
}

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.

Throw, only correct for already sorted data

Comment on lines +1334 to +1336
public AColGroup reduceCols() {
return null;
}

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.

throw

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.

Remove legacy text

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.

Remove DP tests

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.

A few recurring patterns I noticed here:

  • Several tests only verify return types, non-null values, or that unsupported methods throw
  • Some expected results derived from the implementation itself instead of being computed independently
  • Error tolerances sometimes very loose relative to input range
  • Missing edge cases (e.g., non-commutative ops, transposed input, zero target loss, serialization through the normal ColGroupIO)
  • Leftover DP tests

@github-project-automation github-project-automation Bot moved this from In Progress to In Review in SystemDS PR Queue Aug 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Review

Development

Successfully merging this pull request may close these issues.

5 participants