Skip to content

Conversation

guimarqu
Copy link
Contributor

@guimarqu guimarqu commented Jan 29, 2021

Hi,

I started to write tests to close #881.

I create two models. One to test MOI.LessThan(Inf) and MOI.GreaterThan(Inf). Another to test intervals with infinite sides.
I create a test to try intervals with infinite sides.

Do I need to check the solution with test_model_solution ?
I read that it is for when you meet a bug in a solver wrapper.

@guimarqu guimarqu changed the title Tests cases with infinite bounds Test cases with infinite bounds Jan 29, 2021
@odow
Copy link
Member

odow commented Jan 29, 2021

Yes please, add a test for the test.

@guimarqu guimarqu marked this pull request as ready for review January 29, 2021 21:12
Copy link
Member

@odow odow left a comment

Choose a reason for hiding this comment

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

@blegat are you okay with this? It could break a number of the solver-wrappers tests.

@guimarqu could you run the formatter over this please? import JuliaFormatter; format("src/Test/UnitTests/constraints.jl"). There are just a few style things like trailing commas, and ending the file with a new line.

@blegat
Copy link
Member

blegat commented Jan 31, 2021

In the doc, we don't allow infinite values, [lb, Inf) means that Inf is not included, otherwise, it should be written [lb, Inf].

@odow
Copy link
Member

odow commented Jan 31, 2021

We do for intervals: https://jump.dev/MathOptInterface.jl/v0.9/apireference/#MathOptInterface.Interval. Although the math still says [lower, upper] in R, rather than the extended reals.

@blegat
Copy link
Member

blegat commented Feb 1, 2021

Indeed, then we'll need to update the interval bridge so that it does LessThan or GreaterThan constraints in case the bound is infinite. I'm no sure we should allow infinite values for LessThan and GreaterThan though. If it's useful in some application, we could have ExtendedLessThan and ExtendedGreaterThan that solvers can support if they support infinite values, otherwise we can have a bridge from ExtendedLessThan to LessThan and ExtendedGreaterThan to GreaterThan that do not create any constraint if the bound is infinite.

@guimarqu
Copy link
Contributor Author

guimarqu commented Feb 2, 2021

I don't have any idea of applications where LessThan(Inf) or GreaterThan(-Inf) could be useful. I did the tests according to the issue but the documentation is clear : no infinite bounds for these types of constraint. However, MOI didn't throw an error. In JuMP, @constraint(m, x <= Inf) throws an error.

In Coluna, we use LP/MILP solvers in direct mode to optimize subproblems. We always use an Interval to push the bounds of variables in the solvers. Reading your comments, I'm starting to think that pushing infinite bounds is a bad idea. I think we should play safe and use LessThan and/or GreaterThan instead of an Interval. What do you think ?

@blegat
Copy link
Member

blegat commented Feb 2, 2021

In Coluna, we use LP/MILP solvers in direct mode to optimize subproblems.

You mean without any bridge layer ? So you only work with solvers supporting interval constraints ?

Reading your comments, I'm starting to think that pushing infinite bounds is a bad idea.

No I think it's fine for Interval, we even have a bridge from GreaterThan/LessThan to Interval: https://jump.dev/MathOptInterface.jl/dev/apireference/#MathOptInterface.Bridges.Constraint.GreaterToIntervalBridge
Therefore, if you give a GreaterThan constraint, it might be bridged to Interval with an infinite bound anyway.
Of course, depending on the solver, you Interval constraint might be bridged to GreaterThan instead.
If you want to push it, you could check the bridging cost and create the constraint directly in the form with the lowest bridging cost but that is not worth it ;)

@guimarqu
Copy link
Contributor Author

guimarqu commented Feb 2, 2021

You mean without any bridge layer ? So you only work with solvers supporting interval constraints ?

Yes no bridge and I work only with solvers supporting interval constraints (by chance).

No I think it's fine for Interval, we even have a bridge from GreaterThan/LessThan to Interval: https://jump.dev/MathOptInterface.jl/dev/apireference/#MathOptInterface.Bridges.Constraint.GreaterToIntervalBridge
Therefore, if you give a GreaterThan constraint, it might be bridged to Interval with an infinite bound anyway.
Of course, depending on the solver, you Interval constraint might be bridged to GreaterThan instead.
If you want to push it, you could check the bridging cost and create the constraint directly in the form with the lowest bridging cost but that is not worth it ;)

Then it's ok to use Intervals but with a bridge layer and it should not change performance.

I think I should try to update the Interval bridge to be sure of how it works. So, first I'm going to remove tests of LessThan and GreaterThan with infinite bounds. Can I update the bridge in this PR ?

@blegat
Copy link
Member

blegat commented Feb 2, 2021

I think I should try to update the Interval bridge to be sure of how it works. So, first I'm going to remove tests of LessThan and GreaterThan with infinite bounds. Can I update the bridge in this PR ?

Yes, you can use the new test in test/Bridges/Constraint/split_interval.jl

@guimarqu
Copy link
Contributor Author

guimarqu commented Feb 2, 2021

Yes, you can use the new test in test/Bridges/Constraint/split_interval.jl

This file doesn't exist on master.

@blegat
Copy link
Member

blegat commented Feb 3, 2021

This one: https://github.com/jump-dev/MathOptInterface.jl/blob/master/test/Bridges/Constraint/interval.jl

@guimarqu guimarqu marked this pull request as draft February 3, 2021 09:38
@guimarqu
Copy link
Contributor Author

Hi,

To summarize the conversation above, when we create an interval with one infinite side (or both) which is then bridged into F-LessThan and F-GreaterThan constraints, MOI should avoid creating constraints with set LessThan and GreaterThan that has infinite value because the documentation states that these sets don't allow infinity.

@blegat suggested to create two new sets ExtendedLessThan and ExtendedGreaterThan which allow infinity, and a bridge from F-ExtendedLessThan to F-LessThan (same for GreaterThan) that won't create the LessThan constraint if the rhs is infinity.

However, I just read the conversion but do we really need ExtendedLessThan and ExtendedGreaterThan ? I don't have any idea of applications where LessThan(-+Inf) or GreaterThan(-+Inf) could be useful. I would like to be sure before continuing the implementation (I'll try to make some progress on it once a week).

Moreover, I did a first version of the bridge from F-ExtendedLessThan to F-LessThan. What do you think about the type of the attribute : Nothing if the value of ExtendedLessThan is infinite; the constraint index of the LessThan constraint otherwise ?

struct UnextendBridge{
    T,
    F<:MOI.AbstractFunction,
    ES<:MOI.AbstractSet,
    S<:MOI.AbstractSet
} <: AbstractBridge
    non_extended::Union{Nothing, CI{F,S}}
end

I guess that this Union{Nothing,CI} will move into SplitIntervalBridge if we abandon the concept of extended set.

@blegat
Copy link
Member

blegat commented May 31, 2021

do we really need ExtendedLessThan and ExtendedGreaterThan ?

It depends if someone think that it may be useful. I think we can simply have the Union{Nothing, ...} in SplitIntervalBridge and avoid defining these new sets, I don't think that it would be terribly useful.

@odow
Copy link
Member

odow commented Aug 12, 2021

@guimarqu thanks for the effort you put into this.

There are related changes in JuMP which should make it less likely that infinite bounds reach MOI:
jump-dev/JuMP.jl#2634
jump-dev/JuMP.jl#2618

But fixing this properly is going to require adjustments to the SplitIntervalBridge to only add the finite bounds.

Perhaps we should close this for now and try again in a separate PR?

@guimarqu
Copy link
Contributor Author

Hi,

Oh sorry, I didn't see the message of Benoit...

Ok, let's close this PR.

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.

Add tests cases with infinite bounds
3 participants