Matrix compression for federated learning broadcasts#2524
Matrix compression for federated learning broadcasts#2524nirvanjhurreepriv wants to merge 18 commits into
Conversation
ywcb00
left a comment
ywcb00
left a comment
There was a problem hiding this comment.
Thank you very much for the PR. :)
I added some comments inline. Please have a look at them and try to make the corresponding modifications in the code.
Also, please make sure that the other GitHub workflows succeed, especially the Java Tests and the Coding Style workflows.
Thank you and all the best,
David
| * | ||
| * @author Nirvan C. UdaysinghJhurree |
There was a problem hiding this comment.
Author information is already stored by git. No need to add it explicitly to the function header.
| * | ||
| * @author Nirvan C. Udaysingh Jhurree |
| * | ||
| * |
| * | ||
| * |
| * | ||
| * |
| * | ||
| * | ||
| */ | ||
| public class ProbabilisticQuantizationCompressorTest { |
There was a problem hiding this comment.
You can extend from the AutomatedTestBase class and leverage the utility functions in org.apache.sysds.test.TestUtils.
[This applies to both test classes that are added with this PR]
|
|
||
| // Normalize to [0, 1] | ||
| double normalized = (value - min) / (max - min); | ||
| normalized = Math.max(0.0, Math.min(1.0, normalized)); // Clamp |
There was a problem hiding this comment.
Why do we need this line? Can the value be smaller than zero or larger than 1 after normalizing it to [0, 1]?
| // Find bounding level indices | ||
| double scaled = normalized * (levels - 1); | ||
| int lowerIdx = (int) scaled; | ||
| int upperIdx = Math.min(lowerIdx + 1, levels - 1); |
There was a problem hiding this comment.
Do we need the Math.min operation here?
| public org.apache.sysds.runtime.matrix.data.MatrixBlock decompress(CompressedMatrix compressed) | ||
| throws org.apache.sysds.runtime.compress.exceptions.DecompressionException { | ||
| return (org.apache.sysds.runtime.matrix.data.MatrixBlock) compressed.getCompressedData(); |
There was a problem hiding this comment.
Import the MatrixBlock and use it as MatrixBlock instead of writing the full path.
| case ONE_BIT_CS: | ||
| throw new UnsupportedOperationException( | ||
| "1-Bit Compressed Sensing not yet implemented"); |
There was a problem hiding this comment.
Are you still planning on implementing compressed sensing?
If yes, here is a reference that might be useful: https://doi.org/10.1109/CISS.2008.4558487
Otherwise, please remove this case.
github-project-automation
Bot
moved this from In Progress
to In Review
in SystemDS PR Queue
|
@ywcb00 I have addressed all the review feedback in commit fcca7c0. Here is a summary of what was changed:
I have verified locally that checkstyle passes and all 14 tests pass. Could you please approve the workflows so the Java Tests and Coding Style checks can run on your end as well? Best, |
|
@ywcb00 All issues are now fixed in the latest commit: Checkstyle: converted all indentation to tabs The remaining CI failures are pre-existing and unrelated to our changes: 721 unapproved files in docs/ and vendor JS/CSS, and BuiltinMultiLogRegTest.testMultiLogRegInterceptSpark2 timing out after 1000s on the CI runner. Thanks, |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2524 +/- ##
============================================
+ Coverage 71.41% 71.51% +0.10%
- Complexity 48817 49366 +549
============================================
Files 1572 1602 +30
Lines 189089 191045 +1956
Branches 37101 37430 +329
============================================
+ Hits 135030 136627 +1597
- Misses 43596 43884 +288
- Partials 10463 10534 +71 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
Authors (for submission)
Summary
Implements matrix compression techniques to reduce communication overhead in SystemDS's federated learning backend.
Changes
Testing
mvn test -Dtest=TopKCompressorTest,ProbabilisticQuantizationCompressorTest