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 supports_constrained_variable(s) #1004

Merged
merged 5 commits into from
Jan 22, 2020
Merged

Conversation

blegat
Copy link
Member

@blegat blegat commented Jan 18, 2020

See #987

@blegat blegat added this to the v0.9.10 milestone Jan 18, 2020
@codecov-io
Copy link

codecov-io commented Jan 18, 2020

Codecov Report

Merging #1004 into master will increase coverage by 0.01%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1004      +/-   ##
==========================================
+ Coverage   95.19%   95.21%   +0.01%     
==========================================
  Files          99       99              
  Lines       11166    11349     +183     
==========================================
+ Hits        10630    10806     +176     
- Misses        536      543       +7
Impacted Files Coverage Δ
src/constraints.jl 89.28% <100%> (+0.39%) ⬆️
src/Test/contconic.jl 99.28% <100%> (+0.03%) ⬆️
src/variables.jl 57.89% <33.33%> (-4.61%) ⬇️
src/sets.jl 95.65% <0%> (-0.94%) ⬇️
src/FileFormats/LP/LP.jl 97.95% <0%> (+0.31%) ⬆️

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 49180c6...9cb596f. Read the comment docs.

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 recommended calling it supports_add_constrained_variable. I mentioned this in my comment in #987 but probably too subtly.

docs/src/apimanual.md Outdated Show resolved Hide resolved
docs/src/apimanual.md Outdated Show resolved Hide resolved
src/constraints.jl Outdated Show resolved Hide resolved
src/variables.jl Outdated Show resolved Hide resolved
src/variables.jl Outdated Show resolved Hide resolved
src/variables.jl Outdated

## Example

In the standard conic form, the variables are grouped into several cones
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which standard conic form is this? A reference or link to an example (e.g., SCS) might help.

src/variables.jl Outdated
The solvers should then implement
`supports_add_constrained_variables(::Optimizer, ::Type{<:SupportedCones}) = true`
where `SupportedCones` is the union of all cone types that are supported
but it should not implement
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a run-on sentence and a bit confusing. You need a comma before "but". A semi-colon would work also.
"should not implement" doesn't follow from "should return false". The solver could implement it to return false (but that's not necessary because it's the default). Please tidy up the logic.

src/variables.jl Outdated
This prevents the user to constrain the same variable in two different cones.
When a `VectorOfVariables`-in-`S` is added, the variables of the vector
have already been created so they already belong to given cones.
The constraint will therefore be bridged by adding slack variables in `S`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"If bridges are enabled"

src/variables.jl Outdated Show resolved Hide resolved
src/variables.jl Outdated
variables. Then the solver should support adding
`VectorOfVariables`-in-`PositiveSemidefiniteConeTriangle` constraints but it
should not support creating variables constrained to belong to the
`PositiveSemidefiniteConeTriangle` as free variables are not supported.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reword mention of "free variables".
Maybe: "because variables in PositiveSemidefiniteConeTriangle are not necessarily binary or non-negative".

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.

LGTM after minor comments are addressed

docs/src/apimanual.md Outdated Show resolved Hide resolved
@blegat blegat merged commit 68c2882 into master Jan 22, 2020
@odow odow deleted the bl/supports_constrained_variable branch February 20, 2020 22:52
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.

3 participants