-
Notifications
You must be signed in to change notification settings - Fork 1
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
Fast Hadamard Transform #49
base: master
Are you sure you want to change the base?
Conversation
@nathanielpritchard pull request needs links to technical descriptions of the algorithms (referenced papers) |
Project.toml
Outdated
Hadamard = "4a05ff16-5f95-55f4-bb53-bb3f467c689a" | ||
Krylov = "ba0b0d4f-ebba-5204-a429-3ac8c609bfb7" | ||
LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e" | ||
Random = "9a3f8284-a2c9-5f02-9a11-845980a1fd5c" | ||
SparseArrays = "2f01184e-e22b-5df5-ae63-d93ebab69eaf" | ||
StatsBase = "2913bbd2-ae8a-5f71-8c99-4fb6c76f3a91" |
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.
If you add dependencies, there needs to be an explanation in the PR as to why they are there. Also, this branch needs to be updated based on the current master.
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.
I have added a line about the Hadamard
and SparseArrays
decency and remove the StatsBase
dependency.
Merge remote-tracking branch 'refs/remotes/origin/np_fast_hada_trans' into np_fast_hada_trans
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.
@nathanielpritchard we need to be more disciplined about incrementing the version.
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.
Same comments as in proc_block_col_FJLT.jl
test/testgroups.txt
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.
Are you missing the 4 other test files?
RLinearAlgebra.fwht!(x) | ||
@test norm(x - H * xc) < 1e-10 |
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.
Why not test it on the standard basis vectors?
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 standard basis vector I think is somewhat insufficient of a test because of the zeros. For instance the Hadamard package's fwht function I believe would pass the standard unit vector test, but fails with any other vector.
@test norm(x - H * xc .* sc) < 1e-10 | ||
# Test that applying this again gives original vector |
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.
Same comment. Why not test it on the standard basis vectors?
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.
Also order of operations is unclear here given that you are using two distinct types of products. Putting them in parentheses would be better.
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 standard basis vector I think is somewhat insufficient of a test because of the zeros. For instance the Hadamard package's fwht function I believe would pass the standard unit vector test, but fails with any other vector.
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.
I have added the order clarifying parentheses.
# Test with the sign flipping and scaling | ||
x = deepcopy(xc) | ||
RLinearAlgebra.fwht!(x, signs = sgn, scaling = sc) | ||
@test norm(x - H * (xc .* signs) .* sc) < 1e-10 |
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.
Same thing regarding order of operations here.
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.
I have added the order clarifying parentheses
This pull request contains row and column block sketching implementations of SRHT and FJLT transforms.
The SRHT and FJLT are similar in construction with them both requiring the application of a diagonal matrix with random signs to the row or column space of the matrix, followed by a fast Hadamard transform. They differ in the final application with the FJLT applying a sparse matrix with gaussian entries and the SRHT selecting subsets of the row/column indices.
For more information on the FJLT see:
Ailon, Nir, and Bernard Chazelle. "The fast Johnson–Lindenstrauss transform and approximate nearest neighbors." SIAM Journal on computing 39.1 (2009): 302-322.
For more information on the SRHT see:
Tropp, Joel A. "Improved analysis of the subsampled randomized Hadamard transform." Advances in Adaptive Data Analysis 3.01n02 (2011): 115-126.
For the generating the Hadamard matrix using the
hadamard
function, we have added the Hadamard.jl, dependency and for generating the random sparse matrix using thesprand
function we have added the SparseArrays.jl dependency.