add_constrained_variable with 2 sets

[blegat]

Alternative fix for #2564
Actually, this also has the potential to solve another issue.
If you have a variable that belongs to 2 sets, you never know which one should be added at creation as it's solver specific.
When you have a caching optimizer and then a bridge layer it's fine because it will get reordered in copy_to but otherwise you have to manually check the bridging cost.
The implementation of this PR could be improved to handle this case as well by doing something like:

function _cost_of_bridging( # copy pasted from `Utilities/copy.jl`
    dest::MOI.ModelLike,
    ::Type{S},
) where {S<:MOI.AbstractScalarSet}
    x = MOI.get(dest, MOI.VariableBridgingCost{S}())
    y = MOI.get(dest, MOI.ConstraintBridgingCost{MOI.VariableIndex,S}())
    return x - y
end

# ...
cost1 = _cost_of_bridging(model, typeof(set1)
cost2 = _cost_of_bridging(model, typeof(set2)
if cost1 < cost2
    vi, ci1 = MOI.add_constrained_variable(model, set1)
    ci2 = MOI.add_constraint(model, vi, set2)
else
    vi, ci2 = MOI.add_constrained_variable(model, set2)
    ci1 = MOI.add_constraint(model, vi, set1)
end
return vi, ci1, ci2
#...

Closes #2565

[odow]

I really really don't want to make our logic even more complicated than it already is. See jump-dev/JuMP.jl#3863, which involves no changes to MOI, and is "good but not great" and probably sufficient.

[blegat]

If it's sufficient then this PR is not necessary. If it's not sufficient, I thought for a while after the JuMP-dev call and came to the conclusion that this PR might be a better idea than adding the ability to modify individual bounds of a MOI.Interval constraint.

[joaquimg]

I also gave some extra thought to this after the call.
This is, by far, my favorite solution.
It seems clean.

[odow]

I would approve if this was just:

function add_constrained_variable(model::ModelLike, set_gt::GreaterThan, set_lt::LessThan)
    x, _ = add_constrained_variable(model, set_gt)
    add_constraint(model, x, set_lt)
    return x, (set_gt, set_lt)
end

Are there any other instances where someone might want to add constrained variable to two sets (and it makes a performance difference)? I can't think of any.

We also lock in the order of the two sets, otherwise, we need solvers to support permutations, etc.

This single function has a clear use-case up a level (JuMP setting bounds), and it has a clear use-case down a level (HiGHS adding both bounds). This would only find its way to the solver in direct mode, but that's okay.

[odow]

If I add these methods to JuMP and HiGHS, it works as expected:

julia> using JuMP, HiGHS

julia> function main()
           model = direct_model(HiGHS.Optimizer())
           set_silent(model)
           set_string_names_on_creation(model, false)
           @variable(model, 1 <= x[i in 1:10_000] <= 2)
           return model
       end
main (generic function with 1 method)

julia> @time main();
  0.009286 seconds (30.06 k allocations: 2.307 MiB)

julia> using BenchmarkTools

julia> @benchmark main()
BenchmarkTools.Trial: 479 samples with 1 evaluation.
 Range (min … max):   9.022 ms … 278.281 ms  ┊ GC (min … max): 0.00% … 85.28%
 Time  (median):      9.306 ms               ┊ GC (median):    0.00%
 Time  (mean ± σ):   10.433 ms ±  13.956 ms  ┊ GC (mean ± σ):  8.59% ±  6.75%

   ▆▆▇█▅▁                                                       
  ▅███████▅▅▅▂▃▄▃▂▃▃▂▃▃▂▂▃▁▃▃▁▁▂▁▂▁▁▂▂▁▂▁▂▁▁▁▁▁▁▁▂▁▁▁▁▁▁▁▁▁▁▁▂ ▃
  9.02 ms         Histogram: frequency by time         12.6 ms <

 Memory estimate: 2.31 MiB, allocs estimate: 30056.

[odow]

How about:

function add_constrained_variable(
    model::ModelLike,
    set::Tuple{<:GreaterThan,<:LessThan},
)
    set_greater_than, set_less_than = set
    x, c_greater_than = add_constrained_variable(model, set_greater_than)
    c_less_than = add_constraint(model, x, set_less_than)
    return x, (c_greater_than, c_less_than)
end

[odow]

This is one very particular special case because it comes up often. It's not general, or widely supported through the API. It really targets JuMP and models in direct mode.

[blegat]

Yes, if we get a vi, (ci1, ci2) as output, it makes more sense to take a tuple of sets as input as well, I like your last suggestion

[odow]

I'll update this PR.

[odow]

@blegat are you okay with this signature?

[joaquimg]

I have a small preference for two separate arguments rather than the tuple. But if you both prefer the Tuple, let's do it!

[odow]

I feel like the two separate arguments just opens the door too much.

This feels more like a "set" composed of a tuple. Rather than a completely new method. It's more logically consistent with the rest of the API. And the output of the constraint index matches the input of the set. Tuple->Tuple.

[odow]

I also think restricting to this one special case is a good start. We can always relax later if it turns out to be a good idea. But this is really the only case where we've had the need, and it's been years since we started MOI...

[joaquimg]

Fair enough, lets go for it.

[odow]

I continued to think about this all day, and I didn't come up with a reason not to do this.

[blegat]

And the output of the constraint index matches the input of the set.

I agree, otherwise MOI.add_constrained_variable(model, set) should return vi, (ci,) but that would be breaking in addition to being annoying

[blegat]

I can't review my own PR so I mark my validation by this comment