-
Notifications
You must be signed in to change notification settings - Fork 87
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 variable bridges #759
Add variable bridges #759
Conversation
Codecov Report
@@ Coverage Diff @@
## master #759 +/- ##
=======================================
Coverage 94.83% 94.83%
=======================================
Files 71 71
Lines 7643 7643
=======================================
Hits 7248 7248
Misses 395 395 Continue to review full report at Codecov.
|
af74c47
to
0698ddb
Compare
@blegat Do you have a timeline for getting this to a reviewable state? There are a lot of things blocked by the MOI 0.9 release (e.g., transition to new solver wrappers without LQOI, getting rid of |
@mlubin I should be able to finish it in the next few days. I am currently adding doc so that people can start reviewing while I add the tests. |
45291ed
to
20b77f2
Compare
6a99196
to
58d48e6
Compare
Ready from my side. I don't plan any change |
5324966
to
b52eb07
Compare
I will start reviewing this soon. |
I have checked, the branch for MOI v0.9 of the following solvers passes with this branch: GLPK, Mosek, SCS, ECOS, CSDP, SDPA, SeDuMi and CDCS. |
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.
I've looked at only the documentation and minor nits with comments. I don't expect to have major objections to the design; however, this PR is impossibly large to review at 5000+ lines modified. The bus factor for this code would be 1 if I try to review this as is.
I recommend splitting this into smaller PRs. One possible way to do the split is:
- Documentation and stub functions
- Infrastructure and utilities
- A separate PR for each variable bridge
What do you think?
Sounds good, I will first address your comments in this PR to avoid conflicts and will then create smaller PRs.
There will probably not be a good code coverage for each separate PR but as they sum of to this one, it should be ok. |
SGTM |
5e90c4a
to
bd63ad3
Compare
I don't plan to split this PR any further, reviews welcome on this one :) |
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.
@chriscoey, since you're starting to write bridges, could you help review these docs?
98d020b
to
06ab49a
Compare
Yes taking a look now. |
Co-Authored-By: Chris C. <[email protected]>
Co-Authored-By: Chris C. <[email protected]>
Co-Authored-By: Chris C. <[email protected]>
Co-Authored-By: Chris C. <[email protected]>
Co-Authored-By: Chris C. <[email protected]>
Co-Authored-By: Chris C. <[email protected]>
This allows drastic simplifications to SDOI: JuliaOpt/SemidefiniteOptInterface.jl#30 that even questions the reason of the need to keep this SDOI layer as it is rendered trivial by constraint + variable bridges.
default_copy_to
Closes #710
Closes #693