-
Notifications
You must be signed in to change notification settings - Fork 97
[Bridges] add final_touch #1901
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
Conversation
64d360d to
d0caf26
Compare
d0caf26 to
23578ac
Compare
|
I think this The one potential drawback is that there's no priority for final touch. So what if you have two bridges that each needed information about the other? Although I guess that's hidden. I think before we merge this, we need some evidence of actual bridges.
|
odow
left a comment
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.
Are there any situations in which this would be used by a variable or objective bridge? I guess I can't think of any, because it's not like a bridge can support add_constrained_variables(model, AllDifferent(3)).
And the objective bridges already catch modifying ConstraintSet.
|
So I've been playing around with this locally. I think we need to think about it a bit more. We really need a mix of both PRs:
You need the initial notification to throw the right error, but you need the final touch to clean up and fix bounds etc that you don't want to be doing repeatedly. |
b6dc78b to
b1989e5
Compare
|
This seems very promising: julia> using JuMP, HiGHS
[ Info: Precompiling JuMP [4076af6c-e467-56ae-b986-b466b2749572]
[ Info: Precompiling HiGHS [87dc4568-4c63-4d18-b0c0-bb2238e4078b]
julia> model = Model(HiGHS.Optimizer)
A JuMP Model
Feasibility problem with:
Variables: 0
Model mode: AUTOMATIC
CachingOptimizer state: EMPTY_OPTIMIZER
Solver name: HiGHS
julia> set_silent(model)
julia> @variable(model, n, Int)
n
julia> @variable(model, 1 <= x <= 2, Int)
x
julia> @variable(model, 2 <= y <= 3, Int)
y
julia> @constraint(model, [n, x, y] in MOI.CountDistinct(3))
[n, x, y] ∈ MathOptInterface.CountDistinct(3)
julia> @objective(model, Min, x + y)
x + y
julia> optimize!(model)
julia> @show value.([n, x, y])
value.([n, x, y]) = [2.0, 1.0, 2.0]
3-element Vector{Float64}:
2.0
1.0
2.0
julia> @objective(model, Max, x + y)
x + y
julia> optimize!(model)
julia> @show value.([n, x, y])
value.([n, x, y]) = [2.0, 2.0, 3.0]
3-element Vector{Float64}:
2.0
2.0
3.0
julia> @objective(model, Max, x - y)
x - y
julia> optimize!(model)
julia> @show value.([n, x, y])
value.([n, x, y]) = [1.0, 2.0, 2.0]
3-element Vector{Float64}:
1.0
2.0
2.0 |
|
More things working well: a bridge from AllDifferent to CountDistinct, which is a bridge that bridges to a constraint that uses julia> using JuMP, HiGHS
julia> model = Model(HiGHS.Optimizer)
A JuMP Model
Feasibility problem with:
Variables: 0
Model mode: AUTOMATIC
CachingOptimizer state: EMPTY_OPTIMIZER
Solver name: HiGHS
julia> set_silent(model)
julia> @variable(model, 1 <= x <= 2, Int)
x
julia> @variable(model, 2 <= y <= 3, Int)
y
julia> @constraint(model, [x, y] in MOI.AllDifferent(2))
[x, y] ∈ MathOptInterface.AllDifferent(2)
julia> @objective(model, Min, x + y)
x + y
julia> optimize!(model)
julia> value.([x, y])
2-element Vector{Float64}:
1.0
2.0
julia> @objective(model, Max, x + y)
x + y
julia> optimize!(model)
julia> value.([x, y])
2-element Vector{Float64}:
2.0
3.0
julia> @objective(model, Min, x - y)
x - y
julia> optimize!(model)
julia> value.([x, y])
2-element Vector{Float64}:
1.0
3.0 |
|
I'm pretty happy with this. I think it actually solves a nice pain-point in an elegant way. I've done three example bridges without trouble.
All three work and were pretty trivial. There's a weird thing that the inner-most model of the bridge may not correspond to the true model until We might encounter some edge cases once this starts getting used in anger, and the performance impact of rebuilding the reformulation every time might be large. But something is better than nothing. |
|
Here's one edge-case: julia> using JuMP, HiGHS
julia> let
model = Model(HiGHS.Optimizer)
set_silent(model)
@variable(model, 1 <= x <= 2, Int)
@variable(model, 2 <= y <= 3, Int)
@constraint(model, [x, y] in MOI.AllDifferent(2))
@objective(model, Min, x + y)
optimize!(model)
value.([x, y])
end
2-element Vector{Float64}:
1.0
2.0
julia> let
model = Model(HiGHS.Optimizer)
set_silent(model)
@variable(model, 1 <= x <= 2, Int)
@variable(model, 2 <= y <= 3, Int)
@constraint(model, [x + 1, y] in MOI.AllDifferent(2))
@objective(model, Min, x + y)
optimize!(model)
value.([x, y])
end
ERROR: Unable to use CountDistinctToMILPBridge because variable MathOptInterface.VariableIndex(3) has a non-finite domain.
Stacktrace:
[1] error(s::String)
@ Base ./error.jl:33
[2] final_touch(bridge::MathOptInterface.Bridges.Constraint.CountDistinctToMILPBridge{Float64}, model::MathOptInterface.Bridges.LazyBridgeOptimizer{HiGHS.Optimizer})
@ MathOptInterface.Bridges.Constraint ~/.julia/dev/MathOptInterface/src/Bridges/Constraint/bridges/count_distinct.jl:244Because the bridge doesn't support I guess the fix for this is for the bridge to support Edit: The other issue of course is that it is thrown from |
|
This is fixed: julia> using JuMP, HiGHS
julia> let
model = Model(HiGHS.Optimizer)
set_silent(model)
@variable(model, 1 <= x <= 2, Int)
@variable(model, 2 <= y <= 3, Int)
@constraint(model, [x, y] in MOI.AllDifferent(2))
@objective(model, Min, x + y)
optimize!(model)
value.([x, y])
end
2-element Vector{Float64}:
1.0
2.0
julia> let
model = Model(HiGHS.Optimizer)
set_silent(model)
@variable(model, 1 <= x <= 2, Int)
@variable(model, 2 <= y <= 3, Int)
@constraint(model, [x + 1, y] in MOI.AllDifferent(2))
@objective(model, Min, x + y)
optimize!(model)
value.([x, y])
end
2-element Vector{Float64}:
2.0
2.0 |
|
@blegat, I changed this to be |
|
Removed the AllDifferentToCountDistinctBridge into #1923 |
|
I just tagged v1.5.0, so my plan is to merge all of these bridges, but let them sit for a bit to test with MILP solvers before tagging v1.6.0. |
Bridges kind of use the MOI API acting like |
| if MOI.is_valid(model, MOI.ConstraintIndex{F,MOI.EqualTo{T}}(f.value)) | ||
| throw(MOI.LowerBoundAlreadySet{MOI.EqualTo{T},S}(f)) | ||
| end | ||
| if MOI.is_valid(model, MOI.ConstraintIndex{F,MOI.Interval{T}}(f.value)) |
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.
If I understand correctly, this is because Semicontinuous sets bounds so it should be incompatible with existing bounds. In fact, if these bounds are removed before optimize! we shouldn't throw so we should really use final_touch here too. I guess it's another argument for removing these errors as suggested in #1879 (comment)
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.
Maybe put that in another PR if it's not needed to pass the tests so that we can merge the rest
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's going to be a nightmare now that bridges mess up with bounds if we keep this MOI.LowerBoundAlreadySet errors. The fact that bridges can modify these bounds due to the fact that the constraint indices can be inferred from the variable indices is another arguments that these can be muted from everywhere.
| ) where {T,S} | ||
| F, f = MOI.VariableIndex, b.variable | ||
| if MOI.is_valid(model, MOI.ConstraintIndex{F,MOI.GreaterThan{T}}(f.value)) | ||
| throw(MOI.LowerBoundAlreadySet{S,MOI.GreaterThan{T}}(f)) |
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.
If we do this in final_touch we shouldn't do it in bridge_constraint
blegat
left a comment
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 to merge if semi_to_binary is moved to a separate PR as I don't have any comment except for this one.
|
Okay, I'll remove into a separate PR. |
odow
left a comment
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.
Okay, I'll merge once CI is green.
Closes #1665
Closes #1097
What
This PR adds the ability for bridges to implement
Bridges.final_touch. It also implements or improves three bridges as a proof-of-concept to check for correctness of the idea.Why
Some bridges require knowledge about the global state of the model, for example, variable bounds. A good example of this type of bridge is the new
CountDistinctToMILPBridge.final_touchacts as a callback that is run immediately prior tooptimize!, which lets bridges update their reformulation of check that the assumptions are satisfied (e.g., validating other variable bounds).Considerations
There are a few considerations:
ListOfConstraintIndicesandNumberOfVariablesat all times. This safely obscures the difference between the internal state of the bridge and the wider world. Importantly, it means that we don't care about the current internal state of the reformulation at a particular point in time, because this is just an implementation decision.final_touchshould probably calldeleteat the start of the function in order to wipe any previous or partially constructed reformulations. Of course, they could be cleverer and only update the reformulation if the variable bounds have changed etc, but they must not do something like add two reformulations ifoptimize!is called twice in succession.blegat's earlier summary
There are two options:
Keep track on exactly what a bridge depend on and warn them whenever a change is made. This has two issues:
final_touchoroptimize!so now we needfinal_touchoroptimize!to be called to be sure a bridged model is valid. Users that dob.modelmight have a model missing the constraints that want to error.The second option is for bridges to declare that they need
final_touchto be called. Two issues:final_touchoroptimize!to be called for the inner model to be valid but we have the same issue in the first approach as discussed above. We could expose ainner_modelorbridged_modelfunction so that the user has a better way thatfinal_touch(b); b.modelfinal_touchon all bridges that need to even if nothing changed. That's only going to apply to bridges for whichfinal_touchreturnstruethough and that's done in a type-stable way so that might not be such a big issue.This PR implements the second option and #1097 implements the first one.