From aae07d15bf4c368def3d15746f70207eaef09e21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Legat?= Date: Sat, 30 Dec 2023 10:26:56 +0100 Subject: [PATCH 1/5] [Bridges] Throw for bound already set on bridged variable --- src/Bridges/Variable/map.jl | 49 +++++++++++++++++++++++++-- src/Bridges/bridge_optimizer.jl | 8 ++++- test/Bridges/lazy_bridge_optimizer.jl | 5 --- 3 files changed, 54 insertions(+), 8 deletions(-) diff --git a/src/Bridges/Variable/map.jl b/src/Bridges/Variable/map.jl index 30c3af75e2..1d10c3a43b 100644 --- a/src/Bridges/Variable/map.jl +++ b/src/Bridges/Variable/map.jl @@ -44,6 +44,8 @@ mutable struct Map <: AbstractDict{MOI.VariableIndex,AbstractBridge} # `(ci::ConstraintIndex{MOI.VectorOfVariables}).value` -> # the dimension of the set vector_of_variables_length::Vector{Int64} + # Same as in `MOI.Utilities.VariablesContainer` + set_mask::Vector{UInt16} end function Map() @@ -58,6 +60,7 @@ function Map() Dict{MOI.ConstraintIndex,Int64}(), Int64[], Int64[], + UInt16[], ) end @@ -88,6 +91,7 @@ function Base.empty!(map::Map) empty!(map.constraint_context) empty!(map.vector_of_variables_map) empty!(map.vector_of_variables_length) + empty!(map.set_mask) return map end @@ -113,8 +117,9 @@ end function Base.delete!(map::Map, vi::MOI.VariableIndex) if iszero(map.info[-vi.value]) # Delete scalar variable - map.bridges[bridge_index(map, vi)] = nothing - map.sets[bridge_index(map, vi)] = nothing + index = bridge_index(map, vi) + map.bridges[index] = nothing + map.sets[index] = nothing elseif has_keys(map, [vi]) # Delete whole vector delete!(map, [vi]) @@ -131,6 +136,7 @@ function Base.delete!(map::Map, vi::MOI.VariableIndex) map.index_in_vector[i] -= 1 end end + map.set_mask[-vi.value] = MOI.Utilities._DELETED_VARIABLE map.index_in_vector[-vi.value] = -1 return map end @@ -144,6 +150,7 @@ function Base.delete!(map::Map, vis::Vector{MOI.VariableIndex}) ) end for vi in vis + map.set_mask[-vi.value] = MOI.Utilities._DELETED_VARIABLE map.index_in_vector[-vi.value] = -1 end map.bridges[bridge_index(map, first(vis))] = nothing @@ -274,6 +281,41 @@ function MOI.is_valid( map.sets[index] === S end +""" + add_constraint(map::Map, vi::MOI.VariableIndex, S::Type{<:MOI.AbstractScalarSet}) + +Record that a constraint `vi`-in-`S` is added and throws if a lower or upper bound +is set by `S` and such bound has already been set for `vi`. +""" +function add_constraint(::Map, ::MOI.VariableIndex, ::Type{<:MOI.AbstractScalarSet}) + # Nothing to do as this is is not recognized as setting a lower or upper bound +end + +function add_constraint(map::Map, vi::MOI.VariableIndex, ::Type{S}) where {T,S<:MOI.Utilities.SUPPORTED_VARIABLE_SCALAR_SETS{T}} + flag = MOI.Utilities._single_variable_flag(S) + index = -vi.value + mask = map.set_mask[index] + MOI.Utilities._throw_if_lower_bound_set(vi, S, mask, T) + MOI.Utilities._throw_if_upper_bound_set(vi, S, mask, T) + map.set_mask[index] = mask | flag + return +end + +""" + delete(map::Map, ci::MOI.ConstraintIndex{MOI.VariableIndex,<:MOI.AbstractScalarSet}) + +Record that the constraint `vi`-in-`S` is deleted. +""" +function MOI.delete(::Map, ci::MOI.ConstraintIndex{MOI.VariableIndex,<:MOI.AbstractScalarSet}) + # Nothing to do as this is is not recognized as setting a lower or upper bound +end + +function MOI.delete(map::Map, ci::MOI.ConstraintIndex{MOI.VariableIndex,S}) where {T,S<:MOI.Utilities.SUPPORTED_VARIABLE_SCALAR_SETS{T}} + flag = MOI.Utilities._single_variable_flag(S) + map.set_mask[-ci.value] &= ~flag + return +end + """ constraints_with_set(map::Map, S::Type{<:MOI.AbstractSet}) @@ -384,6 +426,7 @@ function add_key_for_bridge( push!(map.index_in_vector, 0) push!(map.bridges, nothing) push!(map.sets, typeof(set)) + push!(map.set_mask, 0x0000) map.bridges[bridge_index] = call_in_context(map, bridge_index, bridge_fun) index = -bridge_index variable = MOI.VariableIndex(index) @@ -443,12 +486,14 @@ function add_keys_for_bridge( push!(map.index_in_vector, 1) push!(map.bridges, nothing) push!(map.sets, typeof(set)) + push!(map.set_mask, 0x0000) for i in 2:MOI.dimension(set) push!(map.parent_index, 0) push!(map.info, i) push!(map.index_in_vector, i) push!(map.bridges, nothing) push!(map.sets, nothing) + push!(map.set_mask, 0x0000) end map.bridges[bridge_index] = call_in_context(map, bridge_index, bridge_fun) variables = MOI.VariableIndex[ diff --git a/src/Bridges/bridge_optimizer.jl b/src/Bridges/bridge_optimizer.jl index 6052b9f5e2..8857798c38 100644 --- a/src/Bridges/bridge_optimizer.jl +++ b/src/Bridges/bridge_optimizer.jl @@ -671,7 +671,7 @@ function MOI.delete(b::AbstractBridgeOptimizer, vi::MOI.VariableIndex) return end -function MOI.delete(b::AbstractBridgeOptimizer, ci::MOI.ConstraintIndex) +function MOI.delete(b::AbstractBridgeOptimizer, ci::MOI.ConstraintIndex{F}) where {F} if is_bridged(b, ci) MOI.throw_if_not_valid(b, ci) br = bridge(b, ci) @@ -687,6 +687,11 @@ function MOI.delete(b::AbstractBridgeOptimizer, ci::MOI.ConstraintIndex) else delete!(Constraint.bridges(b)::Constraint.Map, ci) end + if F === MOI.VariableIndex && ci.value < 0 + # Constraint on a bridged variable so we need to remove the flag + # if it is a bound + MOI.delete(Variable.bridges(b), ci) + end Variable.call_in_context( Variable.bridges(b), ci, @@ -1838,6 +1843,7 @@ function MOI.add_constraint( typeof(f), typeof(s), ) + Variable.add_constraint(Variable.bridges(b), f, typeof(s)) return add_bridged_constraint(b, BridgeType, f, s) end elseif f isa MOI.VectorOfVariables diff --git a/test/Bridges/lazy_bridge_optimizer.jl b/test/Bridges/lazy_bridge_optimizer.jl index d4e72b45e7..5f63e4d083 100644 --- a/test/Bridges/lazy_bridge_optimizer.jl +++ b/test/Bridges/lazy_bridge_optimizer.jl @@ -318,11 +318,6 @@ function test_MOI_runtests_StandardSDPAModel() exclude = Any[MOI.optimize!, MOI.SolverName, MOI.SolverVersion], ); exclude = String[ - # Skip these tests because the bridge reformulates bound - # constraints, so there is no conflict. An error _is_ thrown if two - # sets of the same type are added. - "test_model_LowerBoundAlreadySet", - "test_model_UpperBoundAlreadySet", # MOI.ScalarFunctionConstantNotZero is thrown, not of the original # constraint, but of the bridged constraint. This seems okay. The # fix would require that a bridge optimizer has a try-catch for this From 1819247e4f28c071a977190fa401d1db366efc36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Legat?= Date: Sat, 30 Dec 2023 10:28:32 +0100 Subject: [PATCH 2/5] Fix format --- src/Bridges/Variable/map.jl | 22 ++++++++++++++++++---- src/Bridges/bridge_optimizer.jl | 5 ++++- test/Bridges/lazy_bridge_optimizer.jl | 11 +++++------ 3 files changed, 27 insertions(+), 11 deletions(-) diff --git a/src/Bridges/Variable/map.jl b/src/Bridges/Variable/map.jl index 1d10c3a43b..a412710641 100644 --- a/src/Bridges/Variable/map.jl +++ b/src/Bridges/Variable/map.jl @@ -287,11 +287,19 @@ end Record that a constraint `vi`-in-`S` is added and throws if a lower or upper bound is set by `S` and such bound has already been set for `vi`. """ -function add_constraint(::Map, ::MOI.VariableIndex, ::Type{<:MOI.AbstractScalarSet}) +function add_constraint( + ::Map, + ::MOI.VariableIndex, + ::Type{<:MOI.AbstractScalarSet}, +) # Nothing to do as this is is not recognized as setting a lower or upper bound end -function add_constraint(map::Map, vi::MOI.VariableIndex, ::Type{S}) where {T,S<:MOI.Utilities.SUPPORTED_VARIABLE_SCALAR_SETS{T}} +function add_constraint( + map::Map, + vi::MOI.VariableIndex, + ::Type{S}, +) where {T,S<:MOI.Utilities.SUPPORTED_VARIABLE_SCALAR_SETS{T}} flag = MOI.Utilities._single_variable_flag(S) index = -vi.value mask = map.set_mask[index] @@ -306,11 +314,17 @@ end Record that the constraint `vi`-in-`S` is deleted. """ -function MOI.delete(::Map, ci::MOI.ConstraintIndex{MOI.VariableIndex,<:MOI.AbstractScalarSet}) +function MOI.delete( + ::Map, + ci::MOI.ConstraintIndex{MOI.VariableIndex,<:MOI.AbstractScalarSet}, +) # Nothing to do as this is is not recognized as setting a lower or upper bound end -function MOI.delete(map::Map, ci::MOI.ConstraintIndex{MOI.VariableIndex,S}) where {T,S<:MOI.Utilities.SUPPORTED_VARIABLE_SCALAR_SETS{T}} +function MOI.delete( + map::Map, + ci::MOI.ConstraintIndex{MOI.VariableIndex,S}, +) where {T,S<:MOI.Utilities.SUPPORTED_VARIABLE_SCALAR_SETS{T}} flag = MOI.Utilities._single_variable_flag(S) map.set_mask[-ci.value] &= ~flag return diff --git a/src/Bridges/bridge_optimizer.jl b/src/Bridges/bridge_optimizer.jl index 8857798c38..8e035742a9 100644 --- a/src/Bridges/bridge_optimizer.jl +++ b/src/Bridges/bridge_optimizer.jl @@ -671,7 +671,10 @@ function MOI.delete(b::AbstractBridgeOptimizer, vi::MOI.VariableIndex) return end -function MOI.delete(b::AbstractBridgeOptimizer, ci::MOI.ConstraintIndex{F}) where {F} +function MOI.delete( + b::AbstractBridgeOptimizer, + ci::MOI.ConstraintIndex{F}, +) where {F} if is_bridged(b, ci) MOI.throw_if_not_valid(b, ci) br = bridge(b, ci) diff --git a/test/Bridges/lazy_bridge_optimizer.jl b/test/Bridges/lazy_bridge_optimizer.jl index 5f63e4d083..f4fbf1b340 100644 --- a/test/Bridges/lazy_bridge_optimizer.jl +++ b/test/Bridges/lazy_bridge_optimizer.jl @@ -318,12 +318,11 @@ function test_MOI_runtests_StandardSDPAModel() exclude = Any[MOI.optimize!, MOI.SolverName, MOI.SolverVersion], ); exclude = String[ - # MOI.ScalarFunctionConstantNotZero is thrown, not of the original - # constraint, but of the bridged constraint. This seems okay. The - # fix would require that a bridge optimizer has a try-catch for this - # error. - "test_model_ScalarFunctionConstantNotZero", - ], + # MOI.ScalarFunctionConstantNotZero is thrown, not of the original + # constraint, but of the bridged constraint. This seems okay. The + # fix would require that a bridge optimizer has a try-catch for this + # error. + "test_model_ScalarFunctionConstantNotZero",], ) return end From 61c7c055d8c3ac18af0325056000b43123ce93b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Legat?= Date: Sat, 30 Dec 2023 10:31:41 +0100 Subject: [PATCH 3/5] Small renaming --- src/Bridges/Variable/map.jl | 16 ++++++---------- src/Bridges/bridge_optimizer.jl | 2 +- 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/src/Bridges/Variable/map.jl b/src/Bridges/Variable/map.jl index a412710641..51b49f00c1 100644 --- a/src/Bridges/Variable/map.jl +++ b/src/Bridges/Variable/map.jl @@ -282,23 +282,19 @@ function MOI.is_valid( end """ - add_constraint(map::Map, vi::MOI.VariableIndex, S::Type{<:MOI.AbstractScalarSet}) + MOI.add_constraint(map::Map, vi::MOI.VariableIndex, set::MOI.AbstractScalarSet) -Record that a constraint `vi`-in-`S` is added and throws if a lower or upper bound -is set by `S` and such bound has already been set for `vi`. +Record that a constraint `vi`-in-`set` is added and throws if a lower or upper bound +is set by this constraint and such bound has already been set for `vi`. """ -function add_constraint( - ::Map, - ::MOI.VariableIndex, - ::Type{<:MOI.AbstractScalarSet}, -) +function MOI.add_constraint(::Map, ::MOI.VariableIndex, ::MOI.AbstractScalarSet) # Nothing to do as this is is not recognized as setting a lower or upper bound end -function add_constraint( +function MOI.add_constraint( map::Map, vi::MOI.VariableIndex, - ::Type{S}, + ::S, ) where {T,S<:MOI.Utilities.SUPPORTED_VARIABLE_SCALAR_SETS{T}} flag = MOI.Utilities._single_variable_flag(S) index = -vi.value diff --git a/src/Bridges/bridge_optimizer.jl b/src/Bridges/bridge_optimizer.jl index 8e035742a9..592fdd768a 100644 --- a/src/Bridges/bridge_optimizer.jl +++ b/src/Bridges/bridge_optimizer.jl @@ -1846,7 +1846,7 @@ function MOI.add_constraint( typeof(f), typeof(s), ) - Variable.add_constraint(Variable.bridges(b), f, typeof(s)) + MOI.add_constraint(Variable.bridges(b), f, s) return add_bridged_constraint(b, BridgeType, f, s) end elseif f isa MOI.VectorOfVariables From c6fa3580f789e5d513f939a46e33688ebcfcb7b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Legat?= Date: Sat, 30 Dec 2023 14:19:49 +0100 Subject: [PATCH 4/5] Fix --- src/Bridges/Variable/map.jl | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/Bridges/Variable/map.jl b/src/Bridges/Variable/map.jl index 51b49f00c1..edb993872a 100644 --- a/src/Bridges/Variable/map.jl +++ b/src/Bridges/Variable/map.jl @@ -291,11 +291,24 @@ function MOI.add_constraint(::Map, ::MOI.VariableIndex, ::MOI.AbstractScalarSet) # Nothing to do as this is is not recognized as setting a lower or upper bound end +# We cannot use `SUPPORTED_VARIABLE_SCALAR_SETS` because +# `Integer` and `ZeroOne` do not define `T` and we need `T` +# for `_throw_if_lower_bound_set`. +const _BOUNDED_VARIABLE_SCALAR_SETS{T} = Union{ + MOI.EqualTo{T}, + MOI.GreaterThan{T}, + MOI.LessThan{T}, + MOI.Interval{T}, + MOI.Semicontinuous{T}, + MOI.Semiinteger{T}, + MOI.Parameter{T}, +} + function MOI.add_constraint( map::Map, vi::MOI.VariableIndex, ::S, -) where {T,S<:MOI.Utilities.SUPPORTED_VARIABLE_SCALAR_SETS{T}} +) where {T,S<:_BOUNDED_VARIABLE_SCALAR_SETS{T}} flag = MOI.Utilities._single_variable_flag(S) index = -vi.value mask = map.set_mask[index] @@ -320,7 +333,7 @@ end function MOI.delete( map::Map, ci::MOI.ConstraintIndex{MOI.VariableIndex,S}, -) where {T,S<:MOI.Utilities.SUPPORTED_VARIABLE_SCALAR_SETS{T}} +) where {T,S<:_BOUNDED_VARIABLE_SCALAR_SETS{T}} flag = MOI.Utilities._single_variable_flag(S) map.set_mask[-ci.value] &= ~flag return From 0ad26ef85adf28a7ddb81a1d9989d32e2bba682c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Legat?= Date: Sat, 30 Dec 2023 17:28:58 +0100 Subject: [PATCH 5/5] Add tests --- src/Bridges/Variable/map.jl | 1 + test/Bridges/Variable/map.jl | 15 +++++++++++++++ 2 files changed, 16 insertions(+) diff --git a/src/Bridges/Variable/map.jl b/src/Bridges/Variable/map.jl index edb993872a..3f91b2c9f9 100644 --- a/src/Bridges/Variable/map.jl +++ b/src/Bridges/Variable/map.jl @@ -466,6 +466,7 @@ function add_key_for_bridge( end end end + MOI.add_constraint(map, variable, set) return variable, MOI.ConstraintIndex{MOI.VariableIndex,typeof(set)}(index) end diff --git a/test/Bridges/Variable/map.jl b/test/Bridges/Variable/map.jl index 249fa6099c..609a6c462b 100644 --- a/test/Bridges/Variable/map.jl +++ b/test/Bridges/Variable/map.jl @@ -279,6 +279,21 @@ function test_EmptyMap() return end +function test_double_variable_bound() + map = MOI.Bridges.Variable.Map() + b1 = VariableDummyBridge(1) + set1 = MOI.EqualTo(0.0) + v1, c1 = MOI.Bridges.Variable.add_key_for_bridge(map, () -> b1, set1) + MOI.is_valid(map, c1) + MOI.add_constraint(map, v1, MOI.Integer()) + cint = MOI.ConstraintIndex{typeof(v1),MOI.Integer}(v1.value) + MOI.delete(map, cint) + set2 = MOI.LessThan(1.0) + err = MOI.UpperBoundAlreadySet{typeof(set1),typeof(set2)}(v1) + @test_throws err MOI.add_constraint(map, v1, set2) + return +end + end # module TestVariableMap.runtests()