-
Notifications
You must be signed in to change notification settings - Fork 4
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
MOI to MatOI #7
MOI to MatOI #7
Conversation
Almost the same code as https://github.com/jump-dev/SCS.jl/blob/master/src/MOI_wrapper.jl |
Codecov Report
@@ Coverage Diff @@
## master #7 +/- ##
==========================================
- Coverage 86.14% 82.91% -3.23%
==========================================
Files 3 5 +2
Lines 166 281 +115
==========================================
+ Hits 143 233 +90
- Misses 23 48 +25
Continue to review full report at Codecov.
|
Nice, could you adapt it to be closer to the |
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.
Thanks for this contribution! I have just an overall question: is there some plan to add a function getLinearForm
, similar in spirit as getConicForm
but for linear problems? If so, how hard it would be to adapt the function getConicForm
to the linear case?
we did'nt plan getting the linear case. although it shudn't be much difficult - i tried extracting the matrices given LessThan, EqualTo, GreaterThan constraints |
is there a specific convention on using double underscore for functions? It doesn't read that well |
yep removed them now |
having a hard time moving from get_conic_form to MOI.copy_to. @blegat can u share some example using MOI.copy_to on GeometricConicForm in action |
If I understand correctly, you need something close to function get_conic_form(::Type{T}, src::MOI.ModelLike, con_idx)
dest = ConicForm{T, ...}()
idxmap = MOI.copy_to(dest, src)
return dest
end except that you only need the constraints in |
took a lot of time but using |
src/conic_form.jl
Outdated
trimap(i::Integer, j::Integer) = i < j ? trimap(j, i) : div((i-1)*i, 2) + j | ||
trimapL(i::Integer, j::Integer, n::Integer) = i < j ? trimapL(j, i, n) : i + div((2n-j) * (j-1), 2) | ||
sympackedLtoU(x, n=sympackeddim(length(x))) = _sympackedto(x, n, (i, j) -> trimapL(i, j, n), trimap) | ||
sympackedUtoL(x, n) = _sympackedto(x, n, trimap, (i, j) -> trimapL(i, j, n)) |
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.
Why are these here? This is specific to SCS needing to scale off diagonal entries, do you need this too in DiffOpt?
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.
sorry for the late response.
yes, we need at least this part to scale diagonal entries for a PSD cone. my only source of cross checking the actual values was https://github.com/cvxgrp/diffcp, which accepts/returns matrices in SCS format
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 doesn't make much sense to do it by default for the conic form. I would suggest DiffOpt to use something like
https://github.com/jump-dev/SCS.jl/blob/c1e3a8d5469e9f289dd0b3cdd87245792d266317/src/MOI_wrapper.jl#L147
In the future, if the scaled psd cone is brought back then SCS and DiffOpt could use this without any preprocessing function.
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.
See jump-dev/MathOptInterface.jl#531 for the scaled psd cone
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.
ok got it now. will need to correct the tests accordingly!
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.
should I check the presence of SCS/Mosek solver and edit the matrices accordingly - so that for every solver matrix A is same
Not sure to understand what you mean
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.
forgot to edit the comment 😅 . saw the conic solver info at https://github.com/cvxgrp/cvxpy/tree/master/cvxpy/reductions/solvers/conic_solvers and understood this trimap
thing is specific to SCS and shouldn't be a part of MatOI
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.
@blegat is this PR good to go? or is there anything specific to SCS that I'm missing
Change Vector to AbstracVector in template type (jump-dev#4)
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.
Looks good, thanks for the sustained effort on this PR!
My bad. Took a whole week. But generates matrices!