Skip to content
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

Merged
merged 10 commits into from
Aug 12, 2019
Merged

Add variable bridges #759

merged 10 commits into from
Aug 12, 2019

Conversation

blegat
Copy link
Member

@blegat blegat commented Jun 8, 2019

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.

  • Update default_copy_to
  • Update Allocate-Load API
  • Documentation
  • Tests

Closes #710
Closes #693

@codecov-io
Copy link

codecov-io commented Jun 8, 2019

Codecov Report

Merging #759 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           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.

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

src/Bridges/Constraint/single_bridge_optimizer.jl Outdated Show resolved Hide resolved
src/Bridges/Constraint/single_bridge_optimizer.jl Outdated Show resolved Hide resolved
src/Bridges/Variable/Variable.jl Outdated Show resolved Hide resolved
@blegat blegat force-pushed the bl/variable_bridge branch from af74c47 to 0698ddb Compare June 13, 2019 13:14
@blegat blegat added this to the v0.9 milestone Jun 19, 2019
src/Bridges/Constraint/det.jl Outdated Show resolved Hide resolved
src/Bridges/Constraint/map.jl Outdated Show resolved Hide resolved
src/Bridges/Constraint/map.jl Outdated Show resolved Hide resolved
src/Bridges/Constraint/map.jl Outdated Show resolved Hide resolved
src/Bridges/Constraint/quad_to_soc.jl Outdated Show resolved Hide resolved
src/Bridges/bridge.jl Outdated Show resolved Hide resolved
src/Bridges/bridge.jl Outdated Show resolved Hide resolved
src/Bridges/bridge.jl Outdated Show resolved Hide resolved
src/variables.jl Outdated Show resolved Hide resolved
@mlubin
Copy link
Member

mlubin commented Jul 5, 2019

@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 with_optimizer, etc.).

@blegat
Copy link
Member Author

blegat commented Jul 5, 2019

@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.

@blegat blegat force-pushed the bl/variable_bridge branch from 45291ed to 20b77f2 Compare July 7, 2019 13:47
@blegat blegat marked this pull request as ready for review July 8, 2019 19:33
@blegat blegat force-pushed the bl/variable_bridge branch from 6a99196 to 58d48e6 Compare July 10, 2019 09:46
@blegat
Copy link
Member Author

blegat commented Jul 10, 2019

Ready from my side. I don't plan any change except trying @odow's suggestion: #759 (comment).
Last call for comments :)

@blegat blegat force-pushed the bl/variable_bridge branch from 5324966 to b52eb07 Compare July 11, 2019 10:22
@blegat blegat mentioned this pull request Jul 11, 2019
@mlubin
Copy link
Member

mlubin commented Jul 11, 2019

I will start reviewing this soon.

@blegat blegat mentioned this pull request Jul 12, 2019
@blegat
Copy link
Member Author

blegat commented Jul 12, 2019

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.

Copy link
Member

@mlubin mlubin left a 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:

  1. Documentation and stub functions
  2. Infrastructure and utilities
  3. A separate PR for each variable bridge

What do you think?

docs/src/apireference.md Outdated Show resolved Hide resolved
docs/src/apireference.md Outdated Show resolved Hide resolved
docs/src/apireference.md Outdated Show resolved Hide resolved
docs/src/apireference.md Outdated Show resolved Hide resolved
docs/src/apireference.md Outdated Show resolved Hide resolved
src/Bridges/bridge_optimizer.jl Outdated Show resolved Hide resolved
src/Bridges/bridge_optimizer.jl Outdated Show resolved Hide resolved
src/Bridges/bridge_optimizer.jl Outdated Show resolved Hide resolved
src/Bridges/bridge_optimizer.jl Outdated Show resolved Hide resolved
src/Bridges/lazy_bridge_optimizer.jl Outdated Show resolved Hide resolved
@blegat
Copy link
Member Author

blegat commented Jul 14, 2019

What do you think?

Sounds good, I will first address your comments in this PR to avoid conflicts and will then create smaller PRs.
Probably there will be

  • One for fixing deleting of variables for Model and UniversalFallback
  • One for defined constrained variables
  • One for a variable bridges
  • One for each variable bridge

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.

@mlubin
Copy link
Member

mlubin commented Jul 14, 2019

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

@blegat
Copy link
Member Author

blegat commented Aug 9, 2019

I don't plan to split this PR any further, reviews welcome on this one :)

Copy link
Member

@mlubin mlubin left a 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?

docs/src/apimanual.md Outdated Show resolved Hide resolved
@blegat blegat force-pushed the bl/variable_bridge branch from 98d020b to 06ab49a Compare August 9, 2019 20:00
@chriscoey
Copy link
Contributor

@chriscoey, since you're starting to write bridges, could you help review these docs?

Yes taking a look now.

docs/src/apimanual.md Outdated Show resolved Hide resolved
docs/src/apimanual.md Outdated Show resolved Hide resolved
docs/src/apimanual.md Outdated Show resolved Hide resolved
docs/src/apimanual.md Outdated Show resolved Hide resolved
docs/src/apimanual.md Outdated Show resolved Hide resolved
docs/src/apireference.md Outdated Show resolved Hide resolved
docs/src/apireference.md Outdated Show resolved Hide resolved
docs/src/apireference.md Outdated Show resolved Hide resolved
docs/src/apireference.md Outdated Show resolved Hide resolved
docs/src/apireference.md Outdated Show resolved Hide resolved
@blegat blegat merged commit d9673e8 into master Aug 12, 2019
@odow odow deleted the bl/variable_bridge branch August 29, 2019 14:39
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.

Constrained variables and variable bridges Modify ListOfVariableIndices
5 participants