Add opnorm implementation #375
Add opnorm implementation #375farhadrclass wants to merge 16 commits intoJuliaSmoothOptimizers:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #375 +/- ##
==========================================
- Coverage 95.00% 93.48% -1.52%
==========================================
Files 17 22 +5
Lines 1100 1243 +143
==========================================
+ Hits 1045 1162 +117
- Misses 55 81 +26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tmigot
left a comment
tmigot
left a comment
There was a problem hiding this comment.
Thanks @farhadrclass that will be a great add. I made a first batch of comments
There was a problem hiding this comment.
Pull request overview
This pull request introduces a new opnorm function to efficiently compute the operator 2-norm (largest singular value) for matrices and linear operators. The implementation uses a dispatching strategy that selects LAPACK for small dense matrices, ARPACK for larger matrices with Float32/Float64/ComplexF32/ComplexF64 element types, and TSVD for other element types like BigFloat.
Changes:
- Adds
opnormfunction with type-based dispatch for efficient norm computation - Adds comprehensive tests for Float64 and BigFloat matrix types
- Adds new dependencies: Arpack, GenericLinearAlgebra, and TSVD
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 18 comments.
| File | Description |
|---|---|
| src/utilities.jl | Implements opnorm function with dispatch to opnorm_eig for square matrices and opnorm_svd for rectangular matrices, with fallback to TSVD |
| test/test_opnorm.jl | Adds tests for square and rectangular matrices with Float64 and BigFloat types |
| test/runtests.jl | Integrates new test_opnorm.jl into the test suite |
| Project.toml | Adds Arpack, GenericLinearAlgebra, and TSVD dependencies with version constraints |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 15 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@farhadrclass All tests fail and the branch must be rebased. We also need to hear from @MohamedLaghdafHABIBOULLAH and @MaxenceGollier. |
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
Added Arpack to Project.toml dependencies and compat section. Updated utilities.jl to import Arpack, preparing for usage of its functionality.
Replaces the opnorm function with estimate_opnorm throughout the codebase for clarity. Updates all relevant documentation, exports, and test files to use the new function name, and renames the test file accordingly. Update utilities.jl Update test_estimate_opnorm.jl
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Dominique <dominique.orban@gmail.com>
farhadrclass
force-pushed
the
opNorm_branch
branch
from
05ba478 to
8b0da8c
Compare
Added Arpack, GenericLinearAlgebra, and TSVD as dependencies in Project.toml with corresponding compat entries. Updated test_estimate_opnorm.jl to use consistent spacing and formatting in test assertions.
|
@dpo, Thanks for pointing it out, |
farhadrclass
dismissed
tmigot’s stale review
I can not find what @tmigot said back in Aug 2025, since we have changed many of the code, but the Github doesn't let me to request a review till I apply the changes (which I can not find). So I am dissmiss it now so I can request a new review from Tangi
tmigot
left a comment
tmigot
left a comment
There was a problem hiding this comment.
Thanks @farhadrclass for the PR, it seems that a couple of choices are performance dependent. Could you add a script to the benchmark folder then?
Refactored _estimate_opnorm to add a specialized dispatch for Hermitian and Symmetric matrices with Float/Complex types, improving clarity and correctness. Simplified comments and retry logic in opnorm_eig and opnorm_svd. Expanded and restructured tests to cover type stability, dispatch paths, and edge cases for various matrix types and element types.
|
@tmigot thanks for review, I have added the changes, |
|
@MaxenceGollier @MohamedLaghdafHABIBOULLAH Please review in light of what you've been doing. |
There was a problem hiding this comment.
Hi Farhad, for some reason, I did not see your PR, sorry.
Thanks for the PR in all cases.
In my opinion, using Arpack and TSVD is just a bad idea for R2N (I am guessing this is the reason for this PR); it is very costly, allocates, and adds (potentially unmaintained) dependencies to the project.
Moreover, it lacks robustness, in view of the for attempt = 1:max_attempts loops.
I favor the approach with cheap upper bounds from, for example, #395 and JuliaSmoothOptimizers/RegularizedOptimization.jl#275.
That being said, I am fine with adding an opnorm function in LinearOperators but we should specify that the user should not rely too much on it.
Move Arpack and TSVD out of mandatory deps into weakdeps and add an extension (ext/LinearOperatorsArpackTSVDExt.jl) that provides estimate_opnorm methods when those packages are available. The new extension implements opnorm estimation via ARPACK eigs/svds and TSVD (with small-matrix dense fallbacks, retry/NCV growth logic and ARPACK error handling). The original implementation in src/utilities.jl was removed (left as a stub) and tests/Project.toml/tests updated to load TSVD and Arpack for the test suite.
tmigot
left a comment
tmigot
left a comment
There was a problem hiding this comment.
@farhadrclass One thing that I found weird in this is that we define the new function even for usual matrix type, while the repository LinearOperators.jl should implement things only for linear operators, no ?
Are there other examples in this repository where we define such new API for linear algebra? Is there a function that does something similar in LinearAlgebra ?
4 participants
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