Skip to content

Conversation

@mtanneau
Copy link
Contributor

@mtanneau mtanneau commented Sep 11, 2021

Ideally, we would be using a pure iterative method to estimate the smallest (most negative) eigenvalue of a hermitian matrix A... similar to what they use in cvxpy.

For pointer, I think we would want something like Rayleigh quotient method.
As far as I understand (though I might be wrong here), something similar is implemented in both the scipy method used by cvxpy and IterativeSolvers' invpowm when "shift-invert" mode is enabled (the same operator shows up).

While these are iterative methods, the hidden fact is that every iteration requires a left-product with inv(A - shift * I), which would require either a factorization, or another iterative method embedded in the first.

Thus, I decided to systematically use a factorization, namely, the code will attempt to factorize A + shift*I, and if it fails or finds a negative eigenvalue, A is declared not PSD.
The last hurdle is to use an efficient factorization backend, and I try to use sparse fallbacks as much as possible.
The final fallbacks are as follows:

  • if A is sparse and real-valued, I use LDLFactorizations
  • if A is sparse and Complex{Float32} or Complex{Float64}-valued, I use SuiteSparse
  • everything else uses a dense fallback.

The tolerance for negative eigenvalues is the parameter tol, which is set to the square-root of the machine precision, after conversion to floating-point and real as appropriate.

Copy link
Collaborator

@ericphanson ericphanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mtanneau!

I see LDLFactorizations is LGPL licensed. From some brief reading it seems like that should be fine, but since it’s the first non-MIT license I wonder if any of our users would have an issue with it?

I think Invenia uses or used to use Convex, maybe @nickrobinson251 or @iamed2 knows if the license would be an issue for them?

@codecov
Copy link

codecov bot commented Sep 11, 2021

Codecov Report

Merging #457 (9fb2880) into eph/optional_psd (0c7d23d) will decrease coverage by 0.03%.
The diff coverage is 80.00%.

Impacted file tree graph

@@                 Coverage Diff                  @@
##           eph/optional_psd     #457      +/-   ##
====================================================
- Coverage             92.36%   92.32%   -0.04%     
====================================================
  Files                    83       83              
  Lines                  5120     5134      +14     
====================================================
+ Hits                   4729     4740      +11     
- Misses                  391      394       +3     
Impacted Files Coverage Δ
src/Convex.jl 100.00% <ø> (ø)
src/atoms/second_order_cone/quadform.jl 75.67% <80.00%> (+1.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c7d23d...9fb2880. Read the comment docs.

@mtanneau
Copy link
Contributor Author

I see LDLFactorizations is LGPL licensed

We're only linking against it, so we're fine. The only restriction is that, if someone distributes something that includes the source code (I think a docker image or a compiled executable would qualify), then they have to make the code of LDLFactorizations available.
If your users don't get the library from you (e.g. they use Julia's package manager), then there's no issue because you are not re-distributing the source code of LDLFactorizations.

@iamed2
Copy link
Contributor

iamed2 commented Sep 13, 2021

Invenia used to use Convex, but it also wouldn't have been a problem for us then.

Copy link
Collaborator

@ericphanson ericphanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, thanks both!

@ericphanson ericphanson merged commit 9d81d96 into jump-dev:eph/optional_psd Sep 13, 2021
ericphanson added a commit that referenced this pull request Sep 14, 2021
* add `assume_psd` option to `quadform`

* Use sparse symmetric factorization when possible (#457)

* Use sparse symmetric factorization when possible

* Address review comments

* Test PSD-ness of sparse input

* Fix imports and type conflicts

Co-authored-by: mtanneau <9593025+mtanneau@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants