diff --git a/src/Bridges/Constraint/bridges/SemiToBinaryBridge.jl b/src/Bridges/Constraint/bridges/SemiToBinaryBridge.jl index a3aac980a3..df3c86c3e6 100644 --- a/src/Bridges/Constraint/bridges/SemiToBinaryBridge.jl +++ b/src/Bridges/Constraint/bridges/SemiToBinaryBridge.jl @@ -179,7 +179,6 @@ function MOI.delete(model::MOI.ModelLike, bridge::SemiToBinaryBridge) end MOI.delete(model, bridge.upper_bound_index) MOI.delete(model, bridge.lower_bound_index) - MOI.delete(model, bridge.binary_constraint_index) MOI.delete(model, bridge.binary_variable) return end diff --git a/src/Bridges/bridge_optimizer.jl b/src/Bridges/bridge_optimizer.jl index fff102820a..02598ff409 100644 --- a/src/Bridges/bridge_optimizer.jl +++ b/src/Bridges/bridge_optimizer.jl @@ -476,6 +476,32 @@ function MOI.is_valid( v_map = Variable.bridges(b)::Variable.Map return MOI.is_valid(v_map, ci) else + # This return value has the potential to be a false positive: it + # doesn't discriminate between constraints that the user added, and + # constraints that a bridge added that were themselves bridged. + # + # "Fixing" this particular call has a number of wide reaching + # effects because bridges need this to be "true" so that they can + # query attributes of the constraint from `b`. + # + # In most cases a false positive doesn't matter, because we really + # do support querying stuff about it. And also the user needs some + # way of obtaining the correct index, which they won't have except + # by luck/enumeration. + # + # The main place that this is problematic is when we come to delete + # constraints, and in particular VariableIndex constraints, because we + # triviallly have their `.value` field from the `.value` of the + # VariableIndex. + # + # Instead of fixing everything though, we implement some extra + # checks when deleting, and we leave the false-positive as-is for + # now. If you, future reader, hit this comment while debugging, we + # might need to revisit this decision. + # + # x-ref https://github.com/jump-dev/MathOptInterface.jl/issues/2696 + # x-ref https://github.com/jump-dev/MathOptInterface.jl/issues/2817 + # x-ref https://github.com/jump-dev/MathOptInterface.jl/pull/2818 return haskey(Constraint.bridges(b), ci) end else @@ -489,10 +515,10 @@ function _delete_variables_in_vector_of_variables_constraint( ci::MOI.ConstraintIndex{MOI.VectorOfVariables,S}, ) where {S} func = MOI.get(b, MOI.ConstraintFunction(), ci) - variables = copy(func.variables) - if vis == variables + if vis == func.variables MOI.delete(b, ci) else + variables = copy(func.variables) for vi in vis i = findfirst(isequal(vi), variables) if i !== nothing @@ -504,34 +530,81 @@ function _delete_variables_in_vector_of_variables_constraint( end end end + return end +""" + _is_added_by_bridge( + c_map, + cache::Dict{Any,Set{Int64}}, + ci::MOI.ConstraintIndex{F,S}, + ) where {F,S} + +Return `true` if `ci` was added by one of the bridges in `c_map`. + +For performance reasons, we store the index values associated with +`MOI.ListOfConstraintIndices{F,S}` in `cache` so that we don't have to keep +looping through the bridges. +""" +function _is_added_by_bridge( + c_map, + cache::Dict{Any,Set{Int64}}, + ci::MOI.ConstraintIndex{F,S}, +) where {F,S} + ret = get!(cache, (F, S)) do + set = Set{Int64}() + for bridge in values(c_map) + for ci in MOI.get( + bridge, + MOI.ListOfConstraintIndices{F,S}(), + ) + push!(set, ci.value) + end + end + return set + end::Set{Int64} + return ci.value in ret +end + +""" + _delete_variables_in_variables_constraints( + b::AbstractBridgeOptimizer, + vis::Vector{MOI.VariableIndex}, + ) + +The point of this function is to delete constraints associated with the +variables in `vis`. + +## Warning + +Because of the false positive potential in +`is_valid(::AbstractBridgeOptimizer, MOI.ConstraintIndex)`, we need to ensure +that we delete constraints only if they were not added by a different constraint +bridge, otherwise when we come to delete the parent constraint we'll hit a +runtime error where we have already deleted part of the bridged constraint. + +x-ref https://github.com/jump-dev/MathOptInterface.jl/issues/2817 +x-ref https://github.com/jump-dev/MathOptInterface.jl/pull/2818 +""" function _delete_variables_in_variables_constraints( b::AbstractBridgeOptimizer, vis::Vector{MOI.VariableIndex}, ) c_map = Constraint.bridges(b)::Constraint.Map - # Delete all `MOI.VectorOfVariables` constraints of these variables. - # We reverse for the same reason as for `VariableIndex` below. - # As the iterators are lazy, when the inner bridge constraint is deleted, - # it won't be part of the iteration. - for ci in - Iterators.reverse(Constraint.vector_of_variables_constraints(c_map)) - _delete_variables_in_vector_of_variables_constraint(b, vis, ci) - end - # Delete all `MOI.VariableIndex` constraints of these variables. + cache = Dict{Any,Set{Int64}}() for vi in vis - # If a bridged `VariableIndex` constraints creates a second one, - # then we will delete the second one when deleting the first one hence we - # should not delete it again in this loop. - # For this, we reverse the order so that we encounter the first one first - # and we won't delete the second one since `MOI.is_valid(b, ci)` will be `false`. - for ci in Iterators.reverse(Constraint.variable_constraints(c_map, vi)) - if MOI.is_valid(b, ci) + for ci in Constraint.variable_constraints(c_map, vi) + if !_is_added_by_bridge(c_map, cache, ci) MOI.delete(b, ci) end end end + for ci in Constraint.vector_of_variables_constraints(c_map) + if !_is_added_by_bridge(c_map, cache, ci) + _delete_variables_in_vector_of_variables_constraint(b, vis, ci) + end + end + return end function _delete_variables_in_bridged_objective( diff --git a/test/Bridges/bridge_optimizer.jl b/test/Bridges/bridge_optimizer.jl index f42a12ee1e..4a4c1a3c04 100644 --- a/test/Bridges/bridge_optimizer.jl +++ b/test/Bridges/bridge_optimizer.jl @@ -595,6 +595,7 @@ function test_double_deletion_scalar() @test !MOI.Bridges.is_bridged(model, b2.constraint) MOI.delete(model, x) @test !MOI.is_valid(model, x) + @test MOI.is_empty(model) return end @@ -614,6 +615,7 @@ function test_double_deletion_vector() @test !MOI.Bridges.is_bridged(model, b2.constraint) MOI.delete(model, x) @test all(vi -> !MOI.is_valid(model, vi), x) + @test MOI.is_empty(model) return end @@ -1463,6 +1465,111 @@ function test_BridgeRequiresFiniteDomainError() return end +MOI.Utilities.@model( + Model2817a, + (), + (), + (MOI.Nonnegatives,), + (), + (), + (), + (), + (MOI.VectorAffineFunction,) +); + +function MOI.supports_constraint( + ::Model2817a, + ::Type{MOI.VariableIndex}, + ::Type{S}, +) where {S<:MOI.AbstractScalarSet} + return false +end + +function test_issue_2817a() + inner = Model2817a{Float64}() + model = MOI.Bridges.full_bridge_optimizer(inner, Float64); + x, c = MOI.add_constrained_variable(model, MOI.Interval(0.0, 1.0)); + @test isa( + model.constraint_map[c], + MOI.Bridges.Constraint.IntervalToHyperRectangleBridge, + ) + MOI.delete(model, x) + @test !MOI.is_valid(model, x) + @test MOI.is_empty(model) + @test MOI.is_empty(inner) + @test isempty(model.constraint_map) + return +end + +MOI.Utilities.@model( + Model2817b, + (), + (MOI.LessThan,), + (), + (), + (), + (MOI.ScalarAffineFunction,), + (), + () +); + +function MOI.supports_constraint( + ::Model2817b, + ::Type{MOI.VariableIndex}, + ::Type{S}, +) where {S<:MOI.AbstractScalarSet} + return false +end + +function test_issue_2817b() + inner = Model2817b{Float64}() + model = MOI.Bridges.full_bridge_optimizer(inner, Float64); + x, c = MOI.add_constrained_variables(model, MOI.Nonpositives(1)) + @test isa(model.constraint_map[c], MOI.Bridges.Constraint.ScalarizeBridge) + MOI.delete(model, x) + @test !MOI.is_valid(model, x[1]) + @test MOI.is_empty(model) + @test MOI.is_empty(inner) + @test isempty(model.constraint_map) + return +end + +MOI.Utilities.@model( + Model2817c, + (), + (MOI.GreaterThan, MOI.LessThan), + (), + (), + (), + (MOI.ScalarAffineFunction,), + (), + () +); + +function MOI.supports_constraint( + ::Model2817c, + ::Type{MOI.VariableIndex}, + ::Type{S}, +) where {S<:Union{MOI.ZeroOne,MOI.Semiinteger,MOI.Semicontinuous}} + return false +end + +function test_issue_2817c() + inner = Model2817c{Float64}() + model = MOI.Bridges.full_bridge_optimizer(inner, Float64); + x, c = MOI.add_constrained_variable(model, MOI.Semiinteger(2.0, 3.0)) + @test isa( + model.constraint_map[c], + MOI.Bridges.Constraint.SemiToBinaryBridge, + ) + MOI.delete(model, x) + @test !MOI.is_valid(model, x) + @test MOI.is_empty(model) + @test MOI.is_empty(inner) + @test isempty(model.constraint_map) + return +end + end # module TestBridgeOptimizer.runtests()