Add opnorm implementation #375
Conversation
Introduces a new opnorm function that efficiently computes the operator 2-norm for matrices and linear operators, dispatching to LAPACK, ARPACK, or TSVD as appropriate. Adds comprehensive tests for opnorm covering both Float64 and BigFloat types, and integrates the new test file into the test suite. Update Project.toml
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #375 +/- ##
==========================================
- Coverage 95.00% 91.59% -3.41%
==========================================
Files 17 20 +3
Lines 1100 1202 +102
==========================================
+ Hits 1045 1101 +56
- Misses 55 101 +46 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Thanks @farhadrclass that will be a great add. I made a first batch of comments
| - The estimated operator 2-norm of `B` (largest singular value or eigenvalue in absolute value). | ||
| """ | ||
|
|
||
| function LinearAlgebra.opnorm(B; kwargs...) |
There was a problem hiding this comment.
Same question as before. Do we really want to override the default opnorm from LinearAlgebra?
If someone uses this function in its code and add LinearOperators.jl it will mess everything up.
I think it's fine to import function to extand them, but here you override a default behavior.
I would suggest finding a better name and document the similarity with opnorm
| # Fallback for everything else | ||
| function _opnorm(B, ::Type{T}; kwargs...) where {T} | ||
| _, s, _ = tsvd(B) | ||
| return s[1], true # return largest singular value |
There was a problem hiding this comment.
Can you avoid the documentation like this as it doesn't really provide much info
| function opnorm_eig(B; max_attempts::Int = 3) | ||
| n = size(B, 1) | ||
| # 1) tiny dense Float64: direct LAPACK | ||
| if n ≤ 5 |
| end | ||
|
|
||
| return σ, have_svd | ||
| end |
There was a problem hiding this comment.
You can use the formatter to avoid this
| @@ -0,0 +1,33 @@ | |||
| function test_opnorm() | |||
There was a problem hiding this comment.
Avoid encapsulating test sets in a function
| @@ -31,13 +33,15 @@ AMDGPU = "2" | |||
| CUDA = "5" | |||
| ChainRulesCore = "1" | |||
| FastClosures = "0.2, 0.3" | |||
| GenericLinearAlgebra = "0.3.18" | |||
There was a problem hiding this comment.
Can you expand a bit on why we need these new deps and should we have them by default and not in package extension?
Introduces a new opnorm function that efficiently computes the operator 2-norm for matrices and linear operators, dispatching to LAPACK, ARPACK, or TSVD as appropriate. Adds comprehensive tests for opnorm covering both Float64 and BigFloat types, and integrates the new test file into the test suite.
Update Project.toml