Don't erroneously claim support for Integer variables
#245
Conversation
test/test_algorithm.jl
Outdated
| m = JuMP.Model(test_solver) | ||
|
|
||
| @variable(m, objvar, Int) | ||
| @constraint(m, -10 <= objvar) |
There was a problem hiding this comment.
So only the bounds added when creating the variable are considered bounds.
Is this by design, or am i missing some toggle?
There was a problem hiding this comment.
You are right! As it is, there is no toggle to convert a constraint-based bound into a variable bound. But, after the bound propagation algorithm with presolve_bt = true, a new model is created with deduced bounds on the variables based on constraints. In this sense, this toggle seemed unnecessary. But do you think it would be necessary in some cases?
There was a problem hiding this comment.
You are right! As it is, there is no toggle to convert a constraint-based bound into a variable bound.
Ok, then this is basically ready for the review then.
But, after the bound propagation algorithm with
presolve_bt = true, a new model is created with deduced bounds on the variables based on constraints. In this sense, this toggle seemed unnecessary. But do you think it would be necessary in some cases?
Right. But all that happens after IntegerToZeroOneBridge bridge runs
and encounters variables whose bounds may be specified as constraints,
which means those variables may not have bounds after all,
that are required for IntegerToZeroOneBridge to work in the first place,
which in turn means the whole model still fails to be supported.
There was a problem hiding this comment.
Agree! Before this bridge fails, we could leave this to the user and include an explicit error message: "If known, include variable bounds explicitly instead of constraints" ?
There was a problem hiding this comment.
As tests show, IntegerToZeroOneBridge already produces an error message,
so if anything, said error message should/could be improved on MOI side.
If that wasn't the question, i did not follow what the question was.
There was a problem hiding this comment.
This is not a bound. This is the constraint 1.0 * objvar in GreaterThan(-10.0)
There was a problem hiding this comment.
You need @variable(m, -10 <= objvar <= 20, Int), or set_lower_bound(objvar, -10).
There was a problem hiding this comment.
Right. At this point those are more of a negative tests to demonstrate
that such syntax does not produce bounds needed for this to work.
|
Should this have any documentation changes? |
|
Hm, those failures look strange, i don't think i saw them locally, but i'll re-check. |
|
That error doesn't look related |
|
|
|
I've made a bunch of recent changes, including getting the tests to pass, so are you able to rebase on the latest |
They really are not supported, and the claim is only there to provide a nice diagnostic. But that prevents MOI `IntegerToZeroOneBridge` from triggering, and providing Alpine with transparent support for them. Fixes lanl-ansi#244
|
Rebased, but i didn't run tests. |
odow
changed the title
Integer variablesInteger variables
|
So the upside here is that we will not support bounded integer variables. The downside is that for unbounded integer variables the error message is now worse. |
|
s/not/now/ I suppose that's the expected trade-off here, isn't it. |
|
I'm not quite sure why this hasn't been merged, if this is waiting on something from my side, i'm not aware of that. |
|
@odow we could merge this branch if things look okay to you. |
|
@harshangrjn thank you! |
As discussed in #244,
it is counter-productive for Alpine to declare
Integervariables to be supported,only to, then, produce a nice error message. This prevents
IntegerToZeroOneBridgefrom triggering and actually providing transparent support for them..
But that requires adding support at least for
MOI.is_valid(model::Alpine.Optimizer, ci::MOI.ConstraintIndex{MOI.VariableIndex,S})and
MOI.get(model::Alpine.Optimizer, ::MOI.ConstraintSet, ci::MOI.ConstraintIndex{MOI.VariableIndex,S}),but some other pieces seem missing too.
Fixes #244
Refs. #233