[Bridges] fix supports for VariableIndex: Take II (#1992)

src/Bridges/bridge_optimizer.jl

@@ -1345,16 +1345,55 @@ end
 function MOI.supports(
     b::AbstractBridgeOptimizer,
     attr::MOI.AbstractConstraintAttribute,
-    IndexType::Type{MOI.ConstraintIndex{F,S}},
+    ::Type{MOI.ConstraintIndex{F,S}},
 ) where {F,S}
-    if is_bridged(b, F, S)
-        bridge = Constraint.concrete_bridge_type(b, F, S)
-        return MOI.supports(recursive_model(b), attr, bridge)
-    elseif F == MOI.Utilities.variable_function_type(S) && is_bridged(b, S)
-        bridge = Variable.concrete_bridge_type(b, S)
-        return MOI.supports(recursive_model(b), attr, bridge)
+    # !!! warning
+    #     This function is slightly confusing, because we need to account for
+    #     the different ways in which a constraint might be added.
+    if F == MOI.Utilities.variable_function_type(S)
+        # These are VariableIndex and VectorOfVariable constraints.
+        if is_bridged(b, S)
+            # If S needs to be bridged, it usually means that either there is a
+            # variable bridge, or that there is a free variable followed by a
+            # constraint bridge (i.e., the two cases handled below).
+            #
+            # However, it might be the case, like the tests in
+            # Variable/flip_sign.jl, that the model supports F-in-S constraints,
+            # but force-bridges S sets. If so, we might be in the unusual
+            # situation where we support the attribute if the index was added
+            # via add_constraint, but not if it was added via
+            # add_constrained_variable. Because MOI lacks the ability to tell
+            # which happened just based on the type, we're going to default to
+            # asking the variable bridge, at the risk of a false negative.
+            if is_variable_bridged(b, S)
+                bridge = Variable.concrete_bridge_type(b, S)
+                return MOI.supports(recursive_model(b), attr, bridge)
+            else
+                bridge = Constraint.concrete_bridge_type(b, F, S)
+                return MOI.supports(recursive_model(b), attr, bridge)
+            end
+        else
+            # If S doesn't need to be bridged, it usually means that either the
+            # solver supports add_constrained_variable, or it supports free
+            # variables and add_constraint.
+            #
+            # In some cases, it might be that the solver supports
+            # add_constrained_variable, but ends up bridging add_constraint.
+            # Because MOI lacks the ability to tell which one was called based
+            # on the index type, asking the model might give a false negative
+            # (we support the attribute via add_constrained_variable, but the
+            # bridge doesn't via add_constraint because it will be bridged).
+            return MOI.supports(b.model, attr, MOI.ConstraintIndex{F,S})
+        end
     else
-        return MOI.supports(b.model, attr, IndexType)
+        # These are normal add_constraints, so we just check if they are
+        # bridged.
+        if is_bridged(b, F, S)
+            bridge = Constraint.concrete_bridge_type(b, F, S)
+            return MOI.supports(recursive_model(b), attr, bridge)
+        else
+            return MOI.supports(b.model, attr, MOI.ConstraintIndex{F,S})
+        end
     end
 end
 

test/Bridges/bridge_optimizer.jl

@@ -847,6 +847,91 @@ function test_IssueIpopt333_supports_ConstraintDualStart_VariableIndex()
     return
 end
 
+mutable struct _Issue1992 <: MOI.AbstractOptimizer
+    supports::Bool
+    variables::Int
+    constraints::Int
+    _Issue1992(flag) = new(flag, 0, 0)
+end
+
+function MOI.supports_add_constrained_variables(
+    ::_Issue1992,
+    ::Type{<:Union{MOI.Nonpositives,MOI.Nonnegatives}},
+)
+    return true
+end
+
+function MOI.add_constrained_variables(
+    model::_Issue1992,
+    set::S,
+) where {S<:Union{MOI.Nonpositives,MOI.Nonnegatives}}
+    model.variables += 1
+    ci = MOI.ConstraintIndex{MOI.VectorOfVariables,S}(model.variables)
+    return MOI.VariableIndex.(1:set.dimension), ci
+end
+
+function MOI.add_constraint(
+    model::_Issue1992,
+    ::F,
+    ::S,
+) where {F<:MOI.VectorAffineFunction{Float64},S<:MOI.Nonnegatives}
+    model.constraints += 1
+    return MOI.ConstraintIndex{F,S}(model.constraints)
+end
+
+function MOI.supports_constraint(
+    ::_Issue1992,
+    ::Type{MOI.VectorAffineFunction{Float64}},
+    ::Type{MOI.Nonnegatives},
+)
+    return true
+end
+
+function MOI.supports(
+    model::_Issue1992,
+    ::MOI.ConstraintDualStart,
+    ::Type{MOI.ConstraintIndex{F,MOI.Nonnegatives}},
+) where {F<:MOI.VectorAffineFunction{Float64}}
+    return model.supports
+end
+
+function test_Issue1992_supports_ConstraintDualStart_VariableIndex()
+    # supports should be false
+    model = MOI.Bridges.full_bridge_optimizer(_Issue1992(false), Float64)
+    x, _ = MOI.add_constrained_variables(model, MOI.Nonpositives(1))
+    c = MOI.add_constraint(model, MOI.VectorOfVariables(x), MOI.Nonnegatives(1))
+    @test !MOI.supports(model, MOI.ConstraintDualStart(), typeof(c))
+    # supports should be true
+    model = MOI.Bridges.full_bridge_optimizer(_Issue1992(true), Float64)
+    x, _ = MOI.add_constrained_variables(model, MOI.Nonpositives(1))
+    c = MOI.add_constraint(model, MOI.VectorOfVariables(x), MOI.Nonnegatives(1))
+    # !!! warning
+    #     This test is broken with a false negative. See the discussion in
+    #     PR#1992.
+    @test_broken MOI.supports(model, MOI.ConstraintDualStart(), typeof(c))
+    return
+end
+
+function test_bridge_supports_issue_1992()
+    inner = MOI.Utilities.UniversalFallback(MOI.Utilities.Model{Float64}())
+    model = MOI.Bridges.Variable.NonposToNonneg{Float64}(inner)
+    x = MOI.add_variable(model)
+    c = MOI.add_constraint(
+        model,
+        MOI.VectorOfVariables([x]),
+        MOI.Nonpositives(1),
+    )
+    # !!! warning
+    #     This test is broken with a false negative. (Getting and setting the
+    #     attribute works, even though supports is false) See the discussion in
+    #     PR#1992.
+    @test_broken MOI.supports(model, MOI.ConstraintDualStart(), typeof(c))
+    @test MOI.get(model, MOI.ConstraintDualStart(), c) === nothing
+    MOI.set(model, MOI.ConstraintDualStart(), c, [1.0])
+    @test MOI.get(model, MOI.ConstraintDualStart(), c) == [1.0]
+    return
+end
+
 end  # module
 
 TestBridgeOptimizer.runtests()