Matrix compression for federated learning broadcasts#2524
Matrix compression for federated learning broadcasts#2524nirvanjhurreepriv wants to merge 16 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
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