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 support for solver at optimize with bridges #1626

Merged
merged 9 commits into from
Nov 20, 2018
Merged

Conversation

blegat
Copy link
Member

@blegat blegat commented Nov 16, 2018

Before this PR, the MOI layers on top of the optimizer where:
Bridges -> CachingOptimizer
Now if the optimizer supports_default_copy_to, it is:
CachingOptimizer -> Bridges
otherwise, it is:
CachingOptimizer -> Bridges -> CachingOptimizer

When a model is created without any optimizer we have simply:
CachingOptimizer
and the bridges are added with the optimizer when it is added. This allows bridges to work when the optimizer is added after model creation.

Closes #1504

[2] get(::JuMP.JuMPMOIModel{Float64}, ::MathOptInterface.ObjectiveFunction{MathOptInterface.SingleVariable}) at /home/blegat/.julia/dev/MathOptInterface/src/Utilities/model.jl:259
[3] get at /home/blegat/.julia/dev/MathOptInterface/src/Utilities/universalfallback.jl:105 [inlined]
[4] get at /home/blegat/.julia/dev/MathOptInterface/src/Utilities/cachingoptimizer.jl:436 [inlined]
[1] convert at /home/blegat/.julia/dev/MathOptInterface/src/functions.jl:398 [inlined]
Copy link
Member

Choose a reason for hiding this comment

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

Surely this fails because we're not filtering out the path as well? Or does that happen somewhere else?

Copy link
Member Author

Choose a reason for hiding this comment

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

We are filtering out this output so I was not required to update it.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I missed the .* at the end of the regex. r"^Stacktrace:.*"s might be more clear?

Copy link
Member Author

Choose a reason for hiding this comment

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

If I'm not mistaken, .* doesn't catch newlines

Copy link
Member

Choose a reason for hiding this comment

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

The modifier s on the end makes it. I guess that's just as opaque... nm.

@codecov
Copy link

codecov bot commented Nov 16, 2018

Codecov Report

Merging #1626 into master will increase coverage by 0.4%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #1626     +/-   ##
=========================================
+ Coverage   90.86%   91.26%   +0.4%     
=========================================
  Files          28       28             
  Lines        3743     3984    +241     
=========================================
+ Hits         3401     3636    +235     
- Misses        342      348      +6
Impacted Files Coverage Δ
src/objective.jl 93.33% <ø> (ø) ⬆️
src/JuMP.jl 76.28% <100%> (-0.19%) ⬇️
src/constraints.jl 89.18% <100%> (+0.3%) ⬆️
src/copy.jl 88.88% <100%> (-0.4%) ⬇️
src/optimizer_interface.jl 76.47% <72.72%> (+2.55%) ⬆️
src/macros.jl 92.23% <0%> (+2.33%) ⬆️

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 ef17037...88be967. Read the comment docs.

src/JuMP.jl Outdated
"""
function bridge_constraints(model::Model)
caching_optimizer = backend(model)
if caching_optimizer isa MOIU.CachingOptimizer
Copy link
Member

Choose a reason for hiding this comment

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

Nit: if backend(model) isa MOIU.CachingOptimizer. It's weird call something caching_optimizer if it might not be one.

Copy link
Member Author

Choose a reason for hiding this comment

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

I renamed it moi_backend. The advantage of storing it in a local variable is that inference can use the fact that it is in the first clause of the if to infer that the variable is a CachingOptimizer and we loose a bit of the disadvantage that moi_backend is not concretely typed

src/JuMP.jl Outdated
"""
bridge_constraints(model::Model)

Return a `Bool` indicating whether the model `model` is in manual or automatic
Copy link
Member

Choose a reason for hiding this comment

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

The wording is a bit confusing. Maybe split into cases. When in manual or automatic mode, returns X. When in direct mode, returns false.

end

function MOIU.dropoptimizer!(model::Model)
error_if_direct_mode(model, :dropoptimizer!)
@assert mode(model) != Direct
Copy link
Member

Choose a reason for hiding this comment

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

The assert can be dropped now

end

function MOIU.attachoptimizer!(model::Model)
error_if_direct_mode(model, :attachoptimizer!)
@assert mode(model) != Direct
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@blegat blegat merged commit 0a84f19 into master Nov 20, 2018
@blegat blegat deleted the bl/bridgecache branch December 12, 2018 17:05
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