-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add complex PSD cone #194
Add complex PSD cone #194
Conversation
Great. Thank you @araujoms for your effort on this. I am a bit occupied with my day job this week, but I try to review either one evening or on the weekend. Is there a unit test for the complex cone that you could add? |
Sure, I've added a unit test using the native interface. I could also add one using JuMP, but there doesn't seem to be any. |
I can see one test failure on Julia 1.6 🤔 :
|
Well you could drop support for 1.6. It runs fine on 1.9, and 1.10 will soon be the new LTS anyway. |
As it turns out Julia 1.6 is innocent, I just ran the tests locally, and they went fine. I guess it was just a random instability, I made the test deterministic to avoid that. |
@@ -4,14 +4,15 @@ rng = Random.MersenneTwister(12345) | |||
|
|||
|
|||
|
|||
include("./UnitTests/COSMOTestUtils.jl") | |||
#include("./UnitTests/COSMOTestUtils.jl") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any particular reason why you comment this file out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is already included in runtests.jl
. The duplicate inclusion was generating a lot of warnings about function redefinitions.
It looks good so far. We should also point potential users at this feature in the docs. The section on the PsdConeTriangle is here: COSMO.jl/docs/src/getting_started.md Line 35 in c3a04f0
Either by adding a note under the constraints. Or by adding a sentence to the PsdConeTriangle that one can use complex numbers as well (and linking to the test) |
Done. I added another line to the list of cones in Getting Started, and gave more detail in the docstring. |
Thanks a lot |
My pleasure. Thanks for merging. |
Closes #193.
I changed the typedefs of
PsdConeTriangle
andDensePsdConeTriangle
to accept an extra parameter, which determines whether the cone is real or complex. I generalized all relevant code to handle real or complex matrices. Also I added the new cones to the list of supported by MOI.I did some benchmarking for a simple test problem that computes the minimal eigenvalue of a random complex Hermitian matrix. The orange curve is the times for the native complex version as a function of the dimension, whereas the blue curve refers to the the same problem mapped onto real Hermitian matrices via the usual technique
c -> [real(c) imag(c); -imag(c) real(c)]
:As you can see, the difference is brutal. The code I used for benchmark is the one below:
I've also checked that it works with JuMP with the following code:
I noticed two problems: COSMO often fails to converge, hitting the iteration limit (the peaks of the graph above), and doesn't work for Float32. Both problems were there before my changes, though.
☑️ PR checklist