[Bridges] fix ordering of variables returned by LazyBridgeOptimizer (#2695)

Author: odow

src/Bridges/Variable/map.jl

@@ -691,6 +691,8 @@ Base.keys(::EmptyMap) = MOI.Utilities.EmptyVector{MOI.VariableIndex}()
 
 Base.values(::EmptyMap) = MOI.Utilities.EmptyVector{AbstractBridge}()
 
+Base.iterate(::EmptyMap) = nothing
+
 has_bridges(::EmptyMap) = false
 
 number_of_variables(::EmptyMap) = 0

src/Bridges/bridge_optimizer.jl

@@ -717,12 +717,76 @@ function _get_all_including_bridged(
     b::AbstractBridgeOptimizer,
     attr::MOI.ListOfVariableIndices,
 )
-    list = MOI.get(b.model, attr)
-    if !isempty(Variable.bridges(b))
-        # The conversion from `keys` into a `Vector` happens inside `append!`.
-        append!(list, keys(Variable.bridges(b)))
+    # `inner_to_outer` is going to map variable indices in `b.model` to their
+    # bridged variable indices. If the bridge adds multiple variables, we need
+    # only to map the first variable, and we can skip the rest. To mark this
+    # distinction, the tail variables are set to `nothing`.
+    map = Variable.bridges(b)
+    inner_to_outer = Dict{MOI.VariableIndex,Union{Nothing,MOI.VariableIndex}}()
+    # These are variables which appear in `b` but do NOT appear in `b.model`.
+    # One reason might be the Zero bridge in which they are replaced by `0.0`.
+    user_only_variables = MOI.VariableIndex[]
+    for (user_variable, bridge) in map
+        variables = MOI.get(bridge, MOI.ListOfVariableIndices())
+        if isempty(variables)
+            # This bridge maps `user_variable` to constants in `b.model`. We
+            # still need to report back `user_variable` to the user. In
+            # addition, it might represent the start of a VectorOfVariables
+            # function, so we may need to report multiple variables.
+            push!(user_only_variables, user_variable)
+            n = Variable.length_of_vector_of_variables(map, user_variable)
+            for i in 1:n-1
+                push!(
+                    user_only_variables,
+                    MOI.VariableIndex(user_variable.value - i),
+                )
+            end
+        else
+            # This bridge maps `user_variable` to a list of `variables`. We need
+            # to swap out only the `first` variable. The others can be flagged
+            # with `nothing`. To simplify the first/tail loop, we set
+            # `first(variables)` twice; first to `nothing` and then to
+            # `user_variable`.
+            for bridged_variable in variables
+                inner_to_outer[bridged_variable] = nothing
+            end
+            inner_to_outer[first(variables)] = user_variable
+        end
     end
-    return list
+    # We're about to loop over the variables in `.model`, ordered by when they
+    # were added to the model. We need to return a list of the original
+    # user-space variables in the order that they were added. To do so, we need
+    # to undo any Variable.bridges transformations.
+    ret = MOI.VariableIndex[]
+    for inner_variable in MOI.get(b.model, attr)
+        outer_variable = get(inner_to_outer, inner_variable, missing)
+        if ismissing(outer_variable)
+            # inner_variable does not exist in inner_to_outer, which means that
+            # it is not bridged. Pass through unchanged.
+            push!(ret, inner_variable)
+        elseif isnothing(outer_variable)
+            # inner_variable exists in inner_to_outer, but it is set to `nothing`
+            # which means that it is not the first variable in the bridge. Skip
+            # it because it should be hidden from the user.
+        else
+            # inner_variable exists in inner_to_outer. It must be the first
+            # variable in the bridge. Report it back to the user.
+            push!(ret, outer_variable)
+            # `outer_variable` might represent the start of a VectorOfVariables
+            # if multiple user-variables were bridged. Add them all.
+            n = Variable.length_of_vector_of_variables(map, outer_variable)
+            for i in 1:n-1
+                push!(ret, MOI.VariableIndex(outer_variable.value - i))
+            end
+        end
+    end
+    # Since these were replaced by constants, we don't actually know when they
+    # were added to the model. Tack them on at the end...
+    #
+    # If you, future reader, find this troublesome, come up with a better
+    # solution.
+    append!(ret, user_only_variables)
+    return ret
 end
 
 function _get_all_including_bridged(

src/Test/test_basic_constraint.jl

@@ -219,7 +219,6 @@ function _basic_constraint_test_helper(
     set = _set(T, UntypedS)
     N = MOI.dimension(set)
     x = add_variables_fn(model, N)
-
     constraint_function = _function(T, UntypedF, x)
     @assert MOI.output_dimension(constraint_function) == N
     F, S = typeof(constraint_function), typeof(set)
@@ -235,9 +234,21 @@ function _basic_constraint_test_helper(
     @test MOI.get(model, MOI.NumberOfConstraints{F,S}()) == 1
     _test_attribute_value_type(model, MOI.NumberOfConstraints{F,S}())
     ###
+    ### Test MOI.ListOfConstraintIndices
+    ###
+    c_indices = MOI.get(model, MOI.ListOfConstraintIndices{F,S}())
+    @test c_indices == [c]
+    ###
+    ### Test MOI.ListOfConstraintTypesPresent
+    ###
+    @test (F, S) in MOI.get(model, MOI.ListOfConstraintTypesPresent())
+    ###
     ### Test MOI.is_valid
     ###
     @test MOI.is_valid(model, c)
+    # TODO(odow): we could improve this test by checking `c.value+1` and
+    # `c.value-1` but there is a bug in `LazyBridgeOptimizer`.
+    @test !MOI.is_valid(model, typeof(c)(c.value + 12345))
     ###
     ### Test MOI.ConstraintName
     ###
@@ -287,11 +298,6 @@ function _basic_constraint_test_helper(
         _test_attribute_value_type(model, MOI.ConstraintSet(), c)
     end
     ###
-    ### Test MOI.ListOfConstraintIndices
-    ###
-    c_indices = MOI.get(model, MOI.ListOfConstraintIndices{F,S}())
-    @test length(c_indices) == 1
-    ###
     ### Test MOI.add_constraints
     ###
     if F != MOI.VariableIndex && F != MOI.VectorOfVariables

src/Test/test_conic.jl

@@ -7219,3 +7219,117 @@ function setup_test(
 end
 
 version_added(::typeof(test_conic_NormCone)) = v"1.14.0"
+
+function test_add_constrained_PositiveSemidefiniteConeTriangle(
+    model::MOI.ModelLike,
+    config::Config{T},
+) where {T<:Real}
+    @requires MOI.supports_add_constrained_variables(
+        model,
+        MOI.PositiveSemidefiniteConeTriangle,
+    )
+    @requires _supports(config, MOI.delete)
+    # Add a scalar
+    x = MOI.add_variable(model)
+    @test MOI.get(model, MOI.ListOfVariableIndices()) == [x]
+    @test MOI.is_valid(model, x)
+    @test MOI.get(model, MOI.NumberOfVariables()) == 1
+    # Add a PSD matrix
+    set = MOI.PositiveSemidefiniteConeTriangle(2)
+    X, c = MOI.add_constrained_variables(model, set)
+    F, S = MOI.VectorOfVariables, MOI.PositiveSemidefiniteConeTriangle
+    @test (F, S) in MOI.get(model, MOI.ListOfConstraintTypesPresent())
+    @test MOI.get(model, MOI.ListOfConstraintIndices{F,S}()) == [c]
+    @test MOI.get(model, MOI.NumberOfConstraints{F,S}()) == 1
+    @test MOI.is_valid(model, c)
+    @test !MOI.is_valid(model, typeof(c)(c.value - 1))
+    @test !MOI.is_valid(model, typeof(c)(c.value + 1))
+    @test MOI.get(model, MOI.ListOfVariableIndices()) == [x; X]
+    @test all(MOI.is_valid.(model, [x; X]))
+    @test MOI.get(model, MOI.NumberOfVariables()) == 4
+    # Add and delete a scalar
+    y = MOI.add_variable(model)
+    @test MOI.get(model, MOI.ListOfVariableIndices()) == [x; X; y]
+    @test all(MOI.is_valid.(model, [x; X; y]))
+    @test MOI.get(model, MOI.NumberOfVariables()) == 5
+    MOI.delete(model, y)
+    @test !MOI.is_valid(model, y)
+    @test MOI.get(model, MOI.ListOfVariableIndices()) == [x; X]
+    @test all(MOI.is_valid.(model, [x; X]))
+    @test MOI.get(model, MOI.NumberOfVariables()) == 4
+    # Add and delete another scalar
+    z = MOI.add_variable(model)
+    @test MOI.get(model, MOI.ListOfVariableIndices()) == [x; X; z]
+    @test all(MOI.is_valid.(model, [x; X]))
+    @test MOI.is_valid(model, z)
+    @test MOI.get(model, MOI.NumberOfVariables()) == 5
+    MOI.delete(model, z)
+    @test !MOI.is_valid(model, y)
+    @test !MOI.is_valid(model, z)
+    @test MOI.get(model, MOI.ListOfVariableIndices()) == [x; X]
+    @test all(MOI.is_valid.(model, [x; X]))
+    @test MOI.get(model, MOI.NumberOfVariables()) == 4
+    MOI.delete(model, x)
+    @test !MOI.is_valid(model, x)
+    @test !MOI.is_valid(model, y)
+    @test !MOI.is_valid(model, z)
+    @test MOI.get(model, MOI.ListOfVariableIndices()) == X
+    @test all(MOI.is_valid.(model, X))
+    @test MOI.get(model, MOI.NumberOfVariables()) == 3
+    return
+end
+
+function version_added(
+    ::typeof(test_add_constrained_PositiveSemidefiniteConeTriangle),
+)
+    return v"1.38.1"
+end
+
+function test_add_constrained_PositiveSemidefiniteConeTriangle_VariableName(
+    model::MOI.ModelLike,
+    config::Config{T},
+) where {T<:Real}
+    @requires MOI.supports_add_constrained_variables(
+        model,
+        MOI.PositiveSemidefiniteConeTriangle,
+    )
+    @requires MOI.supports(model, MOI.VariableName(), MOI.VariableIndex)
+    set = MOI.PositiveSemidefiniteConeTriangle(2)
+    X, _ = MOI.add_constrained_variables(model, set)
+    MOI.set(model, MOI.VariableName(), X[1], "x")
+    @test MOI.get(model, MOI.VariableName(), X[1]) == "x"
+    return
+end
+
+function version_added(
+    ::typeof(
+        test_add_constrained_PositiveSemidefiniteConeTriangle_VariableName,
+    ),
+)
+    return v"1.38.1"
+end
+
+function test_add_constrained_PositiveSemidefiniteConeTriangle_VariablePrimalStart(
+    model::MOI.ModelLike,
+    config::Config{T},
+) where {T<:Real}
+    @requires MOI.supports_add_constrained_variables(
+        model,
+        MOI.PositiveSemidefiniteConeTriangle,
+    )
+    @requires MOI.supports(model, MOI.VariablePrimalStart(), MOI.VariableIndex)
+    set = MOI.PositiveSemidefiniteConeTriangle(2)
+    X, _ = MOI.add_constrained_variables(model, set)
+    @test all(isnothing, MOI.get.(model, MOI.VariablePrimalStart(), X))
+    MOI.set.(model, MOI.VariablePrimalStart(), X, [1.0, 0.0, 1.0])
+    @test MOI.get.(model, MOI.VariablePrimalStart(), X) == [1.0, 0.0, 1.0]
+    return
+end
+
+function version_added(
+    ::typeof(
+        test_add_constrained_PositiveSemidefiniteConeTriangle_VariablePrimalStart,
+    ),
+)
+    return v"1.38.1"
+end

test/Bridges/Variable/NonposToNonnegBridge.jl

@@ -40,8 +40,10 @@ function test_NonposToNonneg()
     )
     @test MOI.get(mock, MOI.NumberOfVariables()) == 4
     @test MOI.get(bridged_mock, MOI.NumberOfVariables()) == 4
+    # Variables are ordered
+    #   x in R^1, y in R_-^1, z in R^1, s in R^1
     vis = MOI.get(bridged_mock, MOI.ListOfVariableIndices())
-    y = vis[4]
+    y = vis[2]
     @test y.value == -1
 
     @test MOI.supports(
@@ -90,7 +92,7 @@ function test_NonposToNonneg()
     MOI.set(
         bridged_mock,
         MOI.VariableName(),
-        MOI.get(bridged_mock, MOI.ListOfVariableIndices())[4],
+        MOI.get(bridged_mock, MOI.ListOfVariableIndices())[2],
         "v",
     )
     con_v = MOI.get(