Add opnorm implementation #375

farhadrclass · 2025-08-21T21:15:41Z

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

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

farhadrclass · 2025-08-21T21:16:00Z

@tmigot and @MaxenceGollier

codecov · 2025-08-21T21:21:02Z

Codecov Report

❌ Patch coverage is 20.68966% with 46 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.65%. Comparing base (32dbc5e) to head (09558d4).
⚠️ Report is 49 commits behind head on main.

Files with missing lines	Patch %	Lines
src/utilities.jl	20.68%	46 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #375      +/-   ##
==========================================
- Coverage   95.00%   90.65%   -4.35%     
==========================================
  Files          17       20       +3     
  Lines        1100     1209     +109     
==========================================
+ Hits         1045     1096      +51     
- Misses         55      113      +58

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tmigot

Thanks @farhadrclass that will be a great add. I made a first batch of comments

src/utilities.jl

test/test_opnorm.jl

Project.toml

Added Arpack to Project.toml dependencies and compat section. Updated utilities.jl to import Arpack, preparing for usage of its functionality.

Copilot

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 opnorm function 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.

src/utilities.jl

Copilot · 2026-01-20T16:56:23Z

src/utilities.jl

+import LinearAlgebra: opnorm       
+using GenericLinearAlgebra
+using TSVD
+using Arpack
+export opnorm


The opnorm function is being exported before it is imported from LinearAlgebra. It's better practice to import first and then extend/export. Consider moving the import statement before the export statement.

Copilot · 2026-01-20T16:56:23Z

src/utilities.jl

+function LinearAlgebra.opnorm(B; kwargs...)
+  _opnorm(B, eltype(B); kwargs...)
+end


The function returns a tuple (value, boolean) instead of just the norm value as the standard LinearAlgebra.opnorm would. This inconsistent API design might confuse users who expect the standard opnorm behavior. Consider documenting this difference clearly in the docstring or using a different function name.

src/utilities.jl

Copilot · 2026-01-20T16:56:26Z

src/utilities.jl

+        @warn "Arpack error: $e. Increasing NCV to $ncv and retrying."
+        ncv = min(2 * ncv, n)  # Increase NCV but don't exceed matrix size


The warning message at line 362 displays "Increasing NCV to $ncv" but ncv is updated on line 363 after the warning is printed. The warning should either reference the new ncv value or be moved after the assignment. The same issue exists in opnorm_svd at line 402.

Copilot · 2026-01-20T16:56:26Z

src/utilities.jl

+      if occursin("XYAUPD_Exception", string(e)) && ncv < n
+        @warn "Arpack error: $e. Increasing NCV to $ncv and retrying."
+        ncv = min(2 * ncv, n)  # Increase NCV but don't exceed matrix size


In the error handling logic, the code checks for "XYAUPD_Exception" in the error string, but ARPACK exceptions typically contain "AUPD" (e.g., "dsaupd_", "dnaupd_", "znaupd_"). The substring "XYAUPD_Exception" appears to be a placeholder and may not match actual ARPACK errors. Verify that this error detection pattern is correct.

Suggested change

if occursin("XYAUPD_Exception", string(e)) && ncv < n

@warn "Arpack error: $e. Increasing NCV to $ncv and retrying."

ncv = min(2 * ncv, n) # Increase NCV but don't exceed matrix size

if occursin("AUPD", string(e)) && ncv < n

new_ncv = min(2 * ncv, n) # Increased NCV but don't exceed matrix size

@warn "Arpack error: $e. Increasing NCV to $new_ncv and retrying."

ncv = new_ncv

Copilot · 2026-01-20T16:56:27Z

test/test_opnorm.jl

@@ -0,0 +1,33 @@
+function test_opnorm()


The test file uses LinearOperator but doesn't import it. The test file should have proper imports at the top to be self-contained, even though it's included by runtests.jl which has the imports.

Copilot · 2026-01-20T16:56:27Z

test/test_opnorm.jl

+        @test ok == true
+        @test isapprox(λ, 2.0; rtol=1e-12)
+
+        # 2) Rectangular Float64 via direct LAPACK or ARPACK SVD
+        J_mat = [3.0 0.0 0.0; 0.0 1.0 0.0]
+        J_op = LinearOperator(J_mat)
+        σ, ok_sv = opnorm(J_op)
+        @test ok_sv == true
+        @test isapprox(σ, 3.0; rtol=1e-12)
+
+        # 3) Square BigFloat via TSVD
+        B_mat = Matrix{BigFloat}([2.0 0.0; 0.0 -1.0])
+        B_op = LinearOperator(B_mat)
+        λ_bf, ok_bf = opnorm(B_op)
+        @test ok_bf == true
+        @test isapprox(λ_bf, BigFloat(2); rtol=1e-12)
+
+        # 4) Rectangular BigFloat via rectangular TSVD
+        JR_mat = Matrix{BigFloat}([3.0 0.0 0.0; 0.0 1.0 0.0])
+        JR_op = LinearOperator(JR_mat)
+        σ_bf, ok_bf2 = opnorm(JR_op)
+        @test ok_bf2 == true


The test only validates the returned boolean status but doesn't verify what happens when convergence fails (ok == false). Consider adding assertions or handling for when the second return value is false to ensure the function behavior is well-defined in failure cases.

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

Copilot

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.

src/utilities.jl

Copilot · 2026-01-20T17:43:41Z

src/utilities.jl

+  _, s, _ = tsvd(B)
+  return s[1], true


The TSVD fallback path doesn't include any error handling for the case where tsvd might fail or return an empty singular value array. Consider adding bounds checking or error handling for edge cases such as zero-dimensional or empty matrices.

Suggested change

_, s, _ = tsvd(B)

return s[1], true

m, n = size(B)

# Guard against zero-dimensional or empty matrices

if m == 0 || n == 0

return zero(T), false

end

try

_, s, _ = tsvd(B)

# Ensure we have at least one singular value before indexing

if isempty(s)

return zero(T), false

end

return s[1], true

catch e

@warn "TSVD-based operator norm estimation failed: $e"

return zero(T), false

end

Copilot · 2026-01-20T17:43:43Z

src/utilities.jl

+  end
+
+  # 2) iterative ARPACK
+  nev, ncv = 1, max(20, 2*1 + 1)


The ncv calculation appears incorrect. The expression '21 + 1' always evaluates to 3. This should likely be '2nev + 1' to properly relate ncv to the number of eigenvalues requested, as is standard practice when using ARPACK's eigs function.

Suggested change

nev, ncv = 1, max(20, 2*1 + 1)

nev, ncv = 1, max(20, 2*nev + 1)

src/utilities.jl

test/test_estimate_opnorm.jl

src/utilities.jl

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

farhadrclass

.

farhadrclass · 2026-01-20T18:21:04Z

@tmigot and @dpo I updated the code, and also used AI to code review (to test if AI can help us automate the review)
I am now pushing the changes,

src/utilities.jl

dpo · 2026-01-20T20:30:50Z

src/utilities.jl

+"""
+
+function estimate_opnorm(B; kwargs...)
+  _estimate_opnorm(B, eltype(B); kwargs...)


Is this dispatch really necessary or could you parameterize B{T}?

julia> using LinearAlgebra, SparseArrays julia> A = rand(2, 3); julia> typeof(A) Matrix{Float64} (alias for Array{Float64, 2}) julia> typeof(sparse(A)) SparseMatrixCSC{Float64, Int64} julia> typeof(sparse(A)) <: AbstractMatrix{Float64} true

dpo · 2026-01-20T20:33:02Z

src/utilities.jl

+  _estimate_opnorm(B, eltype(B); kwargs...)
+end
+
+# 1. Fallback for Integer/Generic types (Uses TSVD)


In LinearAlgebra, opnorm is only defined for Real types. We should do the same for operators. Does TSVD even work for a matrix of Ints?

Basically, you always use ARPACK excepts for corner cases. But isn’t TSVD better sometimes?

For TSVD, We did ended up doing the profile and compare and it turn out the allocation and accuracy, ARPACK was better, I think I share the results with you in our Zulip

dpo · 2026-01-20T20:38:02Z

src/utilities.jl

+
+  # Tiny dense optimization
+  if n ≤ tiny_dense_threshold
+    return maximum(abs, eigen(Matrix(B)).values), true


Did you profile for what sizes eigen was faster than whatever opnorm does?

I did, but also if I remember correctly, the ARPACK does not handle small matrices (I think under 5 I got an error)
This is one of the places where I found the issue too https://discourse.julialang.org/t/adjusting-nev-from-to-eigs-on-a-sparse-array-using-arpack/27163

src/utilities.jl

Co-authored-by: Dominique <dominique.orban@gmail.com>

farhadrclass requested a review from dpo August 21, 2025 21:15

Update test_opnorm.jl

a67ac20

tmigot requested changes Aug 22, 2025

View reviewed changes

farhadrclass added 2 commits November 10, 2025 15:55

Add Arpack as dependency and import in utilities.jl

13c04c7

Added Arpack to Project.toml dependencies and compat section. Updated utilities.jl to import Arpack, preparing for usage of its functionality.

Merge branch 'JuliaSmoothOptimizers:main' into opNorm_branch

ad307d5

Copilot AI review requested due to automatic review settings January 20, 2026 16:49

Copilot started reviewing on behalf of farhadrclass January 20, 2026 16:49 View session

Copilot AI reviewed Jan 20, 2026

View reviewed changes

farhadrclass requested a review from Copilot January 20, 2026 17:36

Copilot started reviewing on behalf of farhadrclass January 20, 2026 17:37 View session

Copilot AI reviewed Jan 20, 2026

View reviewed changes

farhadrclass and others added 2 commits January 20, 2026 12:45

Apply suggestions from code review

2f6ccb5

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Apply suggestions from code review

05dac40

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

farhadrclass commented Jan 20, 2026

View reviewed changes

Update utilities.jl

07ab5df

dpo reviewed Jan 20, 2026

View reviewed changes

Update src/utilities.jl

09558d4

Co-authored-by: Dominique <dominique.orban@gmail.com>

		@warn "Arpack error: $e. Increasing NCV to $ncv and retrying."
		ncv = min(2 * ncv, n) # Increase NCV but don't exceed matrix size

-      if occursin("XYAUPD_Exception", string(e)) && ncv < n
-        @warn "Arpack error: $e. Increasing NCV to $ncv and retrying."
-        ncv = min(2 * ncv, n)  # Increase NCV but don't exceed matrix size
+      if occursin("AUPD", string(e)) && ncv < n
+        new_ncv = min(2 * ncv, n)  # Increased NCV but don't exceed matrix size
+        @warn "Arpack error: $e. Increasing NCV to $new_ncv and retrying."
+        ncv = new_ncv

-  _, s, _ = tsvd(B)
-  return s[1], true
+  m, n = size(B)
+  # Guard against zero-dimensional or empty matrices
+  if m == 0 || n == 0
+    return zero(T), false
+  end
+  try
+    _, s, _ = tsvd(B)
+    # Ensure we have at least one singular value before indexing
+    if isempty(s)
+      return zero(T), false
+    end
+    return s[1], true
+  catch e
+    @warn "TSVD-based operator norm estimation failed: $e"
+    return zero(T), false
+  end

	nev, ncv = 1, max(20, 2*1 + 1)
	nev, ncv = 1, max(20, 2*nev + 1)

Add opnorm implementation #375

Are you sure you want to change the base?

Add opnorm implementation #375

Conversation

farhadrclass commented Aug 21, 2025

Uh oh!

farhadrclass commented Aug 21, 2025

Uh oh!

codecov bot commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tmigot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

farhadrclass left a comment

Choose a reason for hiding this comment

Uh oh!

farhadrclass commented Jan 20, 2026

Uh oh!

Uh oh!

dpo Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

dpo Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

dpo Jan 20, 2026

Choose a reason for hiding this comment

codecov bot commented Aug 21, 2025 •

edited

Loading