From f6d376a1e2465f846ad3824ef5204037b601baa7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Legat?= Date: Fri, 22 Dec 2023 20:37:03 +0100 Subject: [PATCH 1/2] [Bridges] Fix deletion corner cases --- src/Bridges/Constraint/bridges/functionize.jl | 11 ++++++++++- src/Bridges/Variable/map.jl | 4 +++- test/Bridges/lazy_bridge_optimizer.jl | 14 ++++++++++++++ 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/src/Bridges/Constraint/bridges/functionize.jl b/src/Bridges/Constraint/bridges/functionize.jl index 3e9f6ffbf8..79584888ce 100644 --- a/src/Bridges/Constraint/bridges/functionize.jl +++ b/src/Bridges/Constraint/bridges/functionize.jl @@ -132,8 +132,17 @@ function MOI.delete( new_func = MOI.Utilities.eachscalar(func)[idx] set = MOI.get(model, MOI.ConstraintSet(), bridge.constraint) new_set = MOI.update_dimension(set, MOI.dimension(set) - 1) - MOI.delete(model, bridge.constraint) + # If we first do `MOI.delete` and then `MOI.add_constraint`, + # there might be an issue of `MOI.delete` ends up deleting a + # bridged variables. + # Indeed, in that case, + # `_delete_variables_in_vector_of_variables_constraint` might + # then try to get `get` `ConstrainFunction` for this bridge + # which will fail since the `bridge.constraint` is no more valid. + # We avoid this issue by calling `add_constraint` first. + old_constraint = bridge.constraint bridge.constraint = MOI.add_constraint(model, new_func, new_set) + MOI.delete(model, old_constraint) return end diff --git a/src/Bridges/Variable/map.jl b/src/Bridges/Variable/map.jl index 8f18f3edb6..30c3af75e2 100644 --- a/src/Bridges/Variable/map.jl +++ b/src/Bridges/Variable/map.jl @@ -550,7 +550,9 @@ That is, the variable indices bridged by this bridge or the bridges that created it will not be unbridged in [`unbridged_function`](@ref). """ function call_in_context(map::Map, bridge_index::Int64, f::Function) - if iszero(bridge_index) + # This is a shortcut that is used in particular in the common case where + # no variable bridge is used. + if iszero(bridge_index) && iszero(map.current_context) return f() end previous_context = map.current_context diff --git a/test/Bridges/lazy_bridge_optimizer.jl b/test/Bridges/lazy_bridge_optimizer.jl index 327f1b2289..d51c3cfa5c 100644 --- a/test/Bridges/lazy_bridge_optimizer.jl +++ b/test/Bridges/lazy_bridge_optimizer.jl @@ -2110,6 +2110,20 @@ function test_objective_conversion_cost(T = Float64) return end +function test_delete_index_in_vector(T::Type = Float64) + model = MOI.instantiate(StandardSDPAModel{T}; with_bridge_type = T) + x = MOI.add_variables(model, 4) + c = MOI.add_constraint(model, x, MOI.Nonpositives(4)) + @test MOI.is_valid(model, c) + MOI.delete(model, x[3]) + @test MOI.is_valid(model, c) + @test MOI.is_valid(model, x[1]) + @test MOI.is_valid(model, x[2]) + @test !MOI.is_valid(model, x[3]) + @test MOI.is_valid(model, x[4]) + @test MOI.get(model, MOI.ConstraintFunction(), c).variables == x[[1, 2, 4]] +end + end # module TestBridgesLazyBridgeOptimizer.runtests() From 8852ca12da5a3af2c0ca21c13e958129e8571efc Mon Sep 17 00:00:00 2001 From: Oscar Dowson Date: Sat, 23 Dec 2023 09:38:40 +1300 Subject: [PATCH 2/2] Apply suggestions from code review --- src/Bridges/Constraint/bridges/functionize.jl | 4 ++-- test/Bridges/lazy_bridge_optimizer.jl | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Bridges/Constraint/bridges/functionize.jl b/src/Bridges/Constraint/bridges/functionize.jl index 79584888ce..7da6c4c025 100644 --- a/src/Bridges/Constraint/bridges/functionize.jl +++ b/src/Bridges/Constraint/bridges/functionize.jl @@ -137,8 +137,8 @@ function MOI.delete( # bridged variables. # Indeed, in that case, # `_delete_variables_in_vector_of_variables_constraint` might - # then try to get `get` `ConstrainFunction` for this bridge - # which will fail since the `bridge.constraint` is no more valid. + # then try to get `get` `ConstraintFunction` for this bridge + # which will fail since the `bridge.constraint` is invalid. # We avoid this issue by calling `add_constraint` first. old_constraint = bridge.constraint bridge.constraint = MOI.add_constraint(model, new_func, new_set) diff --git a/test/Bridges/lazy_bridge_optimizer.jl b/test/Bridges/lazy_bridge_optimizer.jl index d51c3cfa5c..3aa708051f 100644 --- a/test/Bridges/lazy_bridge_optimizer.jl +++ b/test/Bridges/lazy_bridge_optimizer.jl @@ -2122,6 +2122,7 @@ function test_delete_index_in_vector(T::Type = Float64) @test !MOI.is_valid(model, x[3]) @test MOI.is_valid(model, x[4]) @test MOI.get(model, MOI.ConstraintFunction(), c).variables == x[[1, 2, 4]] + return end end # module