feature/Distribution: Strohmer-Vershynin #156
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR implements the L2Norm distribution (Strohmer-Vershynin) for weighted sampling in compressors, where row or column selection probability is proportional to their squared L2 norms.
Changes:
- Added
L2NormandL2NormRecipestructs with complete implementation of distribution interface - Comprehensive test suite covering all distribution functions and edge cases
- Documentation updates for API reference
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/Compressors/Distributions/Strohmer-Vershynin.jl |
Implements L2Norm distribution with probability proportional to row/column norms squared |
test/Compressors/Distributions/Strohmer-Vershynin.jl |
Comprehensive test suite covering constructors, distribution completion, updates, and sampling |
src/Compressors/Distributions.jl |
Includes the new distribution file in the module |
src/RLinearAlgebra.jl |
Exports L2Norm and L2NormRecipe types |
docs/src/api/distributions.md |
Adds L2Norm and L2NormRecipe to API documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| Distribution where the probability of selecting a row (or column) is proportional | ||
| to its squared Euclidean norm, as proposed by | ||
| Strohmer and Vershynin (2009) [strohmer2009randomized](@cite). |
There was a problem hiding this comment.
This can be converted to [strohmer2009randomized](@citet)
| then the sampling occurs without replacement. The default value is `true`. | ||
|
|
||
| # Constructor | ||
|
|
||
| L2Norm(;cardinality=Undef(), replace = true) | ||
|
|
||
| ## Returns | ||
| - A `L2Norm` object. | ||
| """ | ||
| mutable struct L2Norm <: Distribution | ||
| cardinality::Cardinality | ||
| replace::Bool | ||
| end | ||
|
|
||
| function L2Norm(; cardinality = Undef(), replace = true) |
There was a problem hiding this comment.
I suggest making the default value to replace = false
| - `replace::Bool`, an option to replace or not during the sampling process based | ||
| on the given weights. | ||
| - `state_space::Vector{Int64}`, the row/column index set. | ||
| - `weights::ProbabilityWeights`, the weights of each element in the state space, |
There was a problem hiding this comment.
"the weights of..." --> "the probability of..."
| throw(ArgumentError("`L2Norm` cardinality must be specified as `Left()` or `Right()`.\ | ||
| `Undef()` is not allowed in `complete_distribution`.")) |
There was a problem hiding this comment.
| throw(ArgumentError("`L2Norm` cardinality must be specified as `Left()` or `Right()`.\ | |
| `Undef()` is not allowed in `complete_distribution`.")) | |
| throw( | |
| ArgumentError( | |
| "`L2Norm` cardinality must be specified as `Left()` or `Right()`.\ | |
| `Undef()` is not allowed in `complete_distribution`." | |
| ) | |
| ) |
| throw(ArgumentError("`L2Norm` cardinality must be specified as `Left()` or `Right()`.\ | ||
| `Undef()` is not allowed in `update_distribution!`.")) |
There was a problem hiding this comment.
Please adjust this throw statement to match the other.
| @test fieldtypes(L2Norm) == (Cardinality, Bool) | ||
|
|
||
| # Default | ||
| let u = L2Norm() |
There was a problem hiding this comment.
Please put u = L2Norm() on the next line
| @test u.replace == true | ||
| end | ||
|
|
||
| let u2 = L2Norm(cardinality = Left(), replace = false) |
There was a problem hiding this comment.
Please only put parameters of the test on the let line or separated by commas
| # Verify that index 2 (the zero row) is NEVER sampled | ||
| @test 2 ∉ x |
| # Perform sampling | ||
| sample_distribution!(x, ur) | ||
|
|
||
| # Verify that index 2 (the zero col) is NEVER sampled |
There was a problem hiding this comment.
This test is not doing what we want it to do
|
|
||
| sample_distribution!(x, ur) | ||
| @test ur.cardinality == Right() | ||
| @test all(s -> 1 <= s <= 5, x) |
There was a problem hiding this comment.
This test is a good idea as well, but it is not clear to me how we know it is working all the time
Description
Implement the distribution in the paper "A randomized Kaczmarz algorithm with exponential convergence". The distribution of each row (or column) of the matrix to be sketched is proportional to the row (or column) norms of the matrix.
Motivation and Context
Add another distribution can be used in compressor.
How has this been tested
The tests are implemented in "test\Compressors\Distributions\Strohmer-Vershynin.jl".
Please run the following codes in the folder of
RLinearAlgebra.jlto see the test results:Types of changes
Checklists:
Code and Comments
If this PR includes modification to the code base, please select all that apply.
API Documentation
Manual Documentation
Testing
@code_lowered and
@code_typed)