Fix logic in cost_of_bridging

src/Utilities/copy.jl

@@ -347,31 +347,87 @@ function _cost_of_bridging(
     dest::MOI.ModelLike,
     ::Type{S},
 ) where {S<:MOI.AbstractScalarSet}
-    return (
-        MOI.get(dest, MOI.VariableBridgingCost{S}()) -
-        MOI.get(dest, MOI.ConstraintBridgingCost{MOI.VariableIndex,S}()),
-        # In case of ties, we give priority to vector sets. See issue #987.
-        false,
-    )
+    x = MOI.get(dest, MOI.VariableBridgingCost{S}())
+    y = MOI.get(dest, MOI.ConstraintBridgingCost{MOI.VariableIndex,S}())
+    return !iszero(x), x - y, true
 end
 
 function _cost_of_bridging(
     dest::MOI.ModelLike,
     ::Type{S},
 ) where {S<:MOI.AbstractVectorSet}
-    return (
-        MOI.get(dest, MOI.VariableBridgingCost{S}()) -
-        MOI.get(dest, MOI.ConstraintBridgingCost{MOI.VectorOfVariables,S}()),
-        # In case of ties, we give priority to vector sets. See issue #987
-        true,
-    )
+    x = MOI.get(dest, MOI.VariableBridgingCost{S}())
+    y = MOI.get(dest, MOI.ConstraintBridgingCost{MOI.VectorOfVariables,S}())
+    return !iszero(x), x - y, false
 end
 
 """
     sorted_variable_sets_by_cost(dest::MOI.ModelLike, src::MOI.ModelLike)
 
-Returns a `Vector{Type}` of the set types corresponding to `VariableIndex` and
+Return a `Vector{Type}` of the set types corresponding to `VariableIndex` and
 `VectorOfVariables` constraints in the order in which they should be added.
+
+## How the order is computed
+
+The sorting happens in the `_cost_of_bridging` function and has three main
+considerations:
+
+1. First add sets for which the `VariableBridgingCost` is `0`. This ensures that
+   we minimize the number of variable bridges that get added.
+2. Then add sets for which the VariableBridgingCost is smaller than the
+   `ConstraintBridgingCost` so they can get added with
+   `add_constrained_variable(s)`.
+3. Finally, break any remaining ties in favor of `AbstractVectorSet`s. This
+   ensures we attempt to add large blocks of variables (e.g., such as PSD
+   matrices) before we add things like variable bounds.
+
+## Why the order is important
+
+The order is important because some solvers require variables to be added in
+particular order, and the order can also impact the bridging decisions.
+
+We favor adding first variables that won't use variables bridges because then
+the variable constraints on the same variable can still be added as
+`VariableIndex` or `VectorOfVariables` constraints.
+
+If a variable does need variable bridges and is part of another variable
+constraint, then the other variable constraint will be force-bridged into affine
+constraints, so there is a hidden cost in terms of number of additional number
+of bridges that will need to be used.
+
+In fact, if the order does matter (in the sense that changing the order of the
+vector returned by this function leads to a different formulation), it means the
+variable is in at least one other variable constraint. Thus, in a sense we
+could do `x - y + sign(x)`` but `!iszero(x), x - y` is fine.
+
+## Example
+
+A key example is Pajarito. It supports `VariableIndex`-in-`Integer` and
+`VectorAffine`-in-`Nonnegatives`. If the user writes:
+```julia
+@variable(model, x >= 1, Int)
+```
+then we need to add two variable-related constraints:
+ * `VariableIndex`-in-`Integer`
+ * `VariableIndex`-in-`GreaterThan`
+The first is natively supported and the variable and constraint bridging cost is
+0. The second must be bridged to  `VectorAffineFunction`-in-`Nonnegatives` via
+`x - 1 in Nonnegatives(1)`, and the variable and constraint bridging cost is
+`1` in both cases.
+
+If the order is `[Integer, GreaterThan]`, then we add `x ∈ Integer` and
+`x - 1 in Nonnegatives`. Both are natively supported and it only requires a
+single constraint bridge.
+
+If the order is `[GreaterThan, Integer]`, then we add a new variable constrained
+to `y ∈ Nonnegatives` and end up with an expression from the variable bridge of
+`x = y + 1`. Then when we add the Integer constraint, we get `y + 1 in Integer`,
+which is not natively supported. Therefore, we need to add `y + 1 - z ∈ Zeros`
+and `z ∈ Integer`. Oops! This cost an extra variable, a variable bridge of
+`x = y + 1`and a `Zeros` constraint.
+
+Unfortunately, we don't have a good way of computing the updated costs for other
+constraints if a variable bridge is chosen.
 """
 function sorted_variable_sets_by_cost(dest::MOI.ModelLike, src::MOI.ModelLike)
     constraint_types = MOI.get(src, MOI.ListOfConstraintTypesPresent())

test/Utilities/copy.jl

@@ -449,7 +449,7 @@ function test_create_variables_using_supports_add_constrained_variable()
     dest = OrderConstrainedVariablesModel()
     bridged_dest = MOI.Bridges.full_bridge_optimizer(dest, Float64)
     @test MOIU.sorted_variable_sets_by_cost(bridged_dest, src) ==
-          Type[MOI.Zeros, MOI.Nonnegatives, MOI.Nonpositives]
+          Type[MOI.Nonnegatives, MOI.Zeros, MOI.Nonpositives]
     @test MOI.supports_add_constrained_variables(bridged_dest, MOI.Nonnegatives)
     @test MOI.get(bridged_dest, MOI.VariableBridgingCost{MOI.Nonnegatives}()) ==
           0.0
@@ -486,12 +486,12 @@ function test_create_variables_using_supports_add_constrained_variable()
         MOI.ConstraintBridgingCost{MOI.VectorOfVariables,MOI.Zeros}(),
     ) == 2.0
     index_map = MOI.copy_to(bridged_dest, src)
-    @test length(dest.constraintIndices) == 4
+    @test length(dest.constraintIndices) == 6
 
     dest = ReverseOrderConstrainedVariablesModel()
     bridged_dest = MOI.Bridges.full_bridge_optimizer(dest, Float64)
     @test MOIU.sorted_variable_sets_by_cost(bridged_dest, src) ==
-          Type[MOI.Zeros, MOI.Nonpositives, MOI.Nonnegatives]
+          Type[MOI.Nonpositives, MOI.Zeros, MOI.Nonnegatives]
     @test MOI.supports_add_constrained_variables(bridged_dest, MOI.Nonnegatives)
     @test MOI.get(bridged_dest, MOI.VariableBridgingCost{MOI.Nonnegatives}()) ==
           2.0
@@ -528,7 +528,7 @@ function test_create_variables_using_supports_add_constrained_variable()
         MOI.ConstraintBridgingCost{MOI.VectorOfVariables,MOI.Zeros}(),
     ) == 3.0
     index_map = MOI.copy_to(bridged_dest, src)
-    @test length(dest.constraintIndices) == 4
+    @test length(dest.constraintIndices) == 6
 
     # With single variables
     src = MOIU.Model{Float64}()
@@ -896,6 +896,57 @@ function test_copy_of_constraints_passed_as_copy_accross_layers()
     return
 end
 
+struct Optimizer1698_1 <: MOI.AbstractOptimizer end
+
+function MOI.supports_constraint(
+    ::Optimizer1698_1,
+    ::Type{MOI.VariableIndex},
+    ::Type{MOI.Integer},
+)
+    return true
+end
+
+function MOI.supports_constraint(
+    ::Optimizer1698_1,
+    ::Type{<:Union{MOI.VectorOfVariables,MOI.VectorAffineFunction{Float64}}},
+    ::Type{<:Union{MOI.Nonnegatives,MOI.SecondOrderCone}},
+)
+    return true
+end
+
+function test_sorted_variable_sets_by_cost_1()
+    src = MOI.Utilities.Model{Float64}()
+    x = MOI.add_variable(src)
+    y = MOI.add_variables(src, 2)
+    MOI.add_constraint(src, x, MOI.GreaterThan(1.0))
+    MOI.add_constraint(src, x, MOI.Integer())
+    MOI.add_constraint(src, y, MOI.SecondOrderCone(2))
+    dest = MOI.Bridges.full_bridge_optimizer(Optimizer1698_1(), Float64)
+    @test MOI.Utilities.sorted_variable_sets_by_cost(dest, src) ==
+          [MOI.SecondOrderCone, MOI.Integer, MOI.GreaterThan{Float64}]
+    return
+end
+
+struct Optimizer1698_2 <: MOI.AbstractOptimizer end
+
+function MOI.supports_constraint(
+    ::Optimizer1698_2,
+    ::Type{<:Union{MOI.VectorOfVariables,MOI.VectorAffineFunction{Float64}}},
+    ::Type{<:Union{MOI.Nonnegatives,MOI.Nonpositives}},
+)
+    return true
+end
+
+function test_sorted_variable_sets_by_cost_2()
+    src = MOI.Utilities.Model{Float64}()
+    MOI.add_constrained_variables(src, MOI.Nonnegatives(2))
+    MOI.add_constrained_variables(src, MOI.Zeros(2))
+    dest = MOI.Bridges.full_bridge_optimizer(Optimizer1698_2(), Float64)
+    @test MOI.Utilities.sorted_variable_sets_by_cost(dest, src) ==
+          [MOI.Nonnegatives, MOI.Zeros]
+    return
+end
+
 end  # module
 
 TestCopy.runtests()