-
Notifications
You must be signed in to change notification settings - Fork 61
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
Enforce [0,1] bounds on binary variables for LP relaxation #236
Conversation
We also take care if (for whatever reason) the user has already provided lower/upper bounds. This reduces a bit of type information on relax_integrality and enfroce_integrality.
Codecov Report
@@ Coverage Diff @@
## master #236 +/- ##
==========================================
+ Coverage 93.62% 93.73% +0.11%
==========================================
Files 14 14
Lines 1207 1229 +22
==========================================
+ Hits 1130 1152 +22
Misses 77 77
Continue to review full report at Codecov.
|
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.
Good catch. I found this the other day, but I forgot to fix it or open an issue.
examples/LP_relax.jl
Outdated
@SDDP.stageobjective(sp, y) | ||
end | ||
|
||
SDDP.train(pb; iteration_limit=20) |
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.
Instead of this example, let's add a more targeted test to test/user_interface.jl
(I haven't run, so there might be bugs)
@testset "relax_integrality and enforce_integrality" begin
model = SDDP.LinearPolicyGraph(
stages = 2, lower_bound = 0.0,
optimizer = JuMP.with_optimizer(GLPK.Optimizer)
) do sp, t
@variable(sp, x, SDDP.State, initial_value = 2.0)
@variable(sp, b1, Bin)
@variable(sp, 0.2 <= b2, Bin)
@variable(sp, 0.5 <= b3 <= 1.2, Bin)
@stageobjective(sp, b1 + b2 + b2)
end
for node in [model[1], model[2]]
@test JuMP.is_binary(node.subproblem[:b1])
@test !JuMP.has_lower_bound(model.subproblem[:b1])
@test !JuMP.has_upper_bound(model.subproblem[:b1])
@test JuMP.is_binary(node.subproblem[:b2])
@test JuMP.lower_bound(node.subproblem[:b2]) == 0.2
@test !JuMP.has_upper_bound(model.subproblem[:b2])
@test JuMP.is_binary(node.subproblem[:b3])
@test JuMP.lower_bound(node.subproblem[:b3]) == 0.5
@test JuMP.upper_bound(node.subproblem[:b3]) == 1.2
end
binaries, integers = SDDP.relax_integrality(model)
for node in [model[1], model[2]]
@test !JuMP.is_binary(node.subproblem[:b1])
@test JuMP.lower_bound(model.subproblem[:b1]) == 0.0
@test JuMP.upper_bound(model.subproblem[:b1]) == 1.0
@test JuMP.is_binary(node.subproblem[:b2])
@test JuMP.lower_bound(node.subproblem[:b2]) == 0.2
@test JuMP.upper_bound(model.subproblem[:b2]) == 1.0
@test JuMP.is_binary(node.subproblem[:b3])
@test JuMP.lower_bound(node.subproblem[:b3]) == 0.5
@test JuMP.upper_bound(node.subproblem[:b3]) == 1.0
end
SDDP.enforce_integrality(model, binaries, integers)
for node in [model[1], model[2]]
@test JuMP.is_binary(node.subproblem[:b1])
@test !JuMP.has_lower_bound(model.subproblem[:b1])
@test !JuMP.has_upper_bound(model.subproblem[:b1])
@test JuMP.is_binary(node.subproblem[:b2])
@test JuMP.lower_bound(node.subproblem[:b2]) == 0.2
@test !JuMP.has_upper_bound(model.subproblem[:b2])
@test JuMP.is_binary(node.subproblem[:b3])
@test JuMP.lower_bound(node.subproblem[:b3]) == 0.5
@test JuMP.upper_bound(node.subproblem[:b3]) == 1.2
end
end
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 know the rewrite is mostly for binary variables, but should we also exercise the integer part on the test?
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.
Also, should that be on user_interface.jl or on algorithm.jl (since relax/enforce belong to the latter)?
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.
Ah, you got me. Both good points:
- yes, add some tests for integer variables
- yes, in
test/algorithm.jl
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.
Sooooo
GLPK.jl is bugging me. For some reason, it simply ignores the lower bound in @variable(m, 0.2 <= b2, Bin)
, and the test even before calling relax_integrality()
fails. For a non-direct model, as well as for Gurobi models, the bounds are indeed taken into account.
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.
The tests passed on Travis? Or are you talking about something else? What do you mean "ignores"? When you query it you get a different value? Or the optimal solution doesn't respect it?
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.
Yes, the tests passed (as you can see). The only thing is that I had to add the direct_model = false
to the constructor, otherwise they would have (completely) failed.
Re: ignore: during model construction. The tests even before relaxing the integrality constraints fail... Maybe we should open a bug on GLPK.jl, but I have a (much) more pressing one already ;)
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.
There are probably bugs like that. I have a re-write of the GLPK wrapper which should tidy up lots of mistakes: jump-dev/GLPK.jl#101. It should be released soon-ish.
Bump. Do you have time to finish this up? Otherwise I can do it. |
We must replace nothing with NaN, which is also a Float64
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.
Thanks for tidying this up. Just waiting on your GLPK query (and green on Travis) and then I'm happy to merge this.
Thanks @bfpc! |
We also take care if (for whatever reason) the user has already provided
lower/upper bounds.
This reduces a bit of type information on relax_integrality and
enfroce_integrality.
Thanks to @iagoleal