Add QuadtoSOC bridge #483
Add QuadtoSOC bridge #483
Conversation
src/Bridges/quadtosocbridge.jl
Outdated
| function MOI.get(model::MOI.ModelLike, attr::MOI.ConstraintPrimal, | ||
| bridge::QuadtoSOCBridge) | ||
| soc = MOI.get(model, attr, bridge.soc) | ||
| @show soc |
src/Bridges/quadtosocbridge.jl
Outdated
| rmul!(Q, -1) | ||
| end | ||
| @show Q | ||
| U = cholesky(Symmetric(Q)).U |
There was a problem hiding this comment.
How are errors handled if Q is not PSD?
src/Bridges/quadtosocbridge.jl
Outdated
| rmul!(Q, -1) | ||
| end | ||
| @show Q | ||
| U = cholesky(Symmetric(Q)).U |
There was a problem hiding this comment.
I'm concerned about the use of a dense factorization. This is probably the wrong thing to do when Q isn't trivially small. (< 100x100). The fact that this bridge will be applied automatically means that users are going to run into a surprise performance bomb with no feedback about what was wrong.
Codecov Report
@@ Coverage Diff @@
## master #483 +/- ##
==========================================
- Coverage 95.64% 95.59% -0.05%
==========================================
Files 46 47 +1
Lines 4682 4839 +157
==========================================
+ Hits 4478 4626 +148
- Misses 204 213 +9
Continue to review full report at Codecov.
|
|
@mlubin All your comments have been addressed :) |
|
It would be cool if Julia could automatically use sparse cholesky if Q is very sparse but dense cholesky otherwise. There has been discussion of making the dense and sparse factorization interfaces fully consistent but no one has stepped up to the plate yet. |
|
I also got confused by the inconsistency. For sparse cholesky, there is a permutation matrix and you have |
| The check for existence of bound can be implemented (but inefficiently) with the | ||
| current interface but if bound is removed or transformed (e.g. `≤ 0` transformed | ||
| into `≥ 0`) then the bridge is no longer valid. For this reason the homogeneous | ||
| version of the bridge is not implemented yet. |
There was a problem hiding this comment.
This sentence at the end is the only hint in this docstring about what the bridge actually does. Say more explicitly at the beginning which transformations are implemented.
src/Bridges/quadtosocbridge.jl
Outdated
| " constraint of type: `$(typeof(func))`-in-`$(typeof(set))`.", | ||
| " A bridge attempted to transform the quadratic constraint", | ||
| " to a second order cone constraint but the constraint is not", | ||
| " convex.") |
There was a problem hiding this comment.
This is incorrect. cholesky will fail when Q is only positive semi-definite, but that's a perfectly valid convex constraint.
There was a problem hiding this comment.
You could try LDLT instead right? Or bunch-kaufman if dense
src/Bridges/quadtosocbridge.jl
Outdated
| " convex.") | ||
| " to a second order cone constraint but the constraint is", | ||
| " not strongly convex, i.e., the symmetric matrix of" | ||
| " quadratic coefficients is not positive definite.") |
There was a problem hiding this comment.
This wording suggests that the user did something wrong, while the issue is in the implementation of the bridge instead. Maybe "... not yet supported" is more appropriate.
Currently, it only bridges
x^T Q x + a^T x + b <= cfor PSD matrix Q and does not supportx^T Q xwith one negative eigenvalue, the reason is discussed in the docstring.Closes JuliaOpt/MathOptInterfaceBridges.jl#93