Project: S3779 LZW ColGroup | ASML - #2560
Conversation
…nd downstream CLA operations
|
Your code does not build (see error logs). Could you please fix those issues to have a prototype that compiles? |
|
@MasterBrain2000 @m-ollka @Mancer1 could you please address the issues causing the tests to fail? Thanks |
|
@janniklinde, all the build problems have been addressed. |
Codecov Report❌ Patch coverage is 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. 🚀 New features to boost your workflow:
|
|
@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. |
|
@janniklinde, How much code coverage is accepted? We have some null functions that are required to be implemented but never called. |
|
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 Also, please avoid reimplementing helper functionality that is already available in the test utilities, such as random matrix generation. |
…o improve coverage
# Conflicts: # src/main/java/org/apache/sysds/runtime/compress/colgroup/ColGroupPiecewiseLinearCompressed.java # src/test/java/org/apache/sysds/test/component/compress/colgroup/ColGroupPiecewiseLinearCompressedOperationsTest.java
|
@janniklinde, we have improved the coverage to 95%. I hope that suffices |
|
@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? |
|
You can simply run the following @Mancer1: |
@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
left a comment
There was a problem hiding this comment.
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.
| /** | ||
| * 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 | ||
| */ |
There was a problem hiding this comment.
Comment not up to date, either remove or update and put above class definition
| 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); | ||
| } |
There was a problem hiding this comment.
These comments are unnecessary. I'd prefer a clear documentation comment for the function itself.
| /** | ||
| * 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; | ||
| } |
There was a problem hiding this comment.
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 | ||
| */ | ||
|
|
| 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); | ||
|
|
||
| } |
There was a problem hiding this comment.
Incorrect if cs.transposed = true, ther e you need to iterate over rows
| public AColGroup sort() { | ||
| return this; | ||
| } |
There was a problem hiding this comment.
Throw, only correct for already sorted data
| public AColGroup reduceCols() { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
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
Group Project for the AMLS Module