-
Notifications
You must be signed in to change notification settings - Fork 1
Version 0.2 Compressor: Gaussian Compression Method #94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
src/Compressors/Gaussian.jl
Outdated
## Returns | ||
- A `Gaussian` object. | ||
""" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No white space
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, a nice implementation. There are a few details that need to be changed to align with SparseSign, and a few stylistic changes requested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor changes. Otherwise, the PR looks good. Some of the comments are about file naming conventions that should be discussed with Nathaniel.
src/Compressors/Gaussian.jl
Outdated
""" | ||
Gaussian <: Compressor | ||
|
||
An implementation of the Gaussian compression method. This method forms a Gaussian sketch matrix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is beyond the allowed character limit
src/Compressors/Gaussian.jl
Outdated
- `compression_dim::Int64`, the target compression dimension. | ||
- `n_rows::Int64`, the number of rows of the compression matrix. | ||
- `n_cols::Int64`, the number of columns of the compression matrix. | ||
- `scale::Number`, the standard deviation of Gaussian distribution during the compression matrix generation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is beyond the character limit
src/Compressors/Gaussian.jl
Outdated
Gaussian <: Compressor | ||
|
||
An implementation of the Gaussian compression method. This method forms a Gaussian sketch matrix | ||
with number of rows or number of columns setting as the compression dimension. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A simple description may be better. E.g., A specification for a Gaussian compressor.
src/Compressors/Gaussian.jl
Outdated
end | ||
|
||
function Gaussian(; | ||
cardinality::Cardinality=Left(), compression_dim::Int64=2, type::Type{<:Number}=Float64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The arguments should be on individual lines to stay with the Blue
style.
src/Compressors/Gaussian.jl
Outdated
# Check on the compression dimension | ||
function Gaussian(cardinality, compression_dim, type) | ||
if compression_dim <= 0 | ||
throw(ArgumentError("Field 'Compression_dim' must be positive.")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compresion_dim
-> compression_dim
src/Compressors/Gaussian.jl
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this file name shouldb elower case to keep with the style of sparse_sign.jl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed into lower case
test/Compressors/Gaussian.jl
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the file name be lower case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed into lower case
test/Compressors/Gaussian.jl
Outdated
typeof(compressor.cardinality) == Card | ||
end | ||
|
||
for type in [Bool, Int16, Int32, Int64, Float16, Float32, Float64] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably do not want to test Bool
or Integers, since this should not be allowed since we cannot generate a Gaussian random variable of this type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted Bool and integers.
The pull request includes two new files:
(1) Compressors/Gaussian.jl': Includes implementation of structures and multiplication functions for Gaussian compression method.
(2) test/Compressors/Gaussian.jl: Includes the tests for the multiplication functions, the data structures and the update/complete functions.