From 3758cbce9b6a51d17a9779f2a5d5f51e9331846c Mon Sep 17 00:00:00 2001 From: odow Date: Tue, 14 Sep 2021 16:44:57 +1200 Subject: [PATCH 1/5] Fix modifications in bridge_optimizer --- src/Bridges/bridge_optimizer.jl | 41 +++++++++++++++++++++++++++++++- src/Test/test_modification.jl | 26 ++++++++++++++++++++ src/Test/test_objective.jl | 26 -------------------- test/Bridges/bridge_optimizer.jl | 12 ++++++++++ 4 files changed, 78 insertions(+), 27 deletions(-) diff --git a/src/Bridges/bridge_optimizer.jl b/src/Bridges/bridge_optimizer.jl index 1f4da83852..1c8a15149a 100644 --- a/src/Bridges/bridge_optimizer.jl +++ b/src/Bridges/bridge_optimizer.jl @@ -1110,6 +1110,9 @@ function MOI.set( ci::MOI.ConstraintIndex{F}, func::F, ) where {F} + if F == MOI.VariableIndex + throw(MOI.SettingVariableIndexNotAllowed()) + end if Variable.has_bridges(Variable.bridges(b)) set = MOI.get(b, MOI.ConstraintSet(), ci) func, new_set = bridged_constraint_function(b, func, set) @@ -1170,7 +1173,7 @@ function MOI.set( ) where {S<:MOI.AbstractScalarSet} if Variable.has_bridges(Variable.bridges(b)) func = MOI.get(b, MOI.ConstraintFunction(), ci) - new_func, set = bridged_constraint_function(b, func, set) + _, set = bridged_constraint_function(b, func, set) end _set_substituted(b, attr, ci, set) return @@ -1241,6 +1244,42 @@ function MOI.supports( ) end +function _set_substituted( + b::AbstractBridgeOptimizer, + ::MOI.ConstraintSet, + ci::MOI.ConstraintIndex{F,S}, + value, +) where {F,S} + if !(value isa S) + throw(ArgumentError("Invalid type when setting ConstraintSet.")) + elseif is_bridged(b, ci) + MOI.throw_if_not_valid(b, ci) + call_in_context(MOI.set, b, ci, attr, value) + else + MOI.set(b.model, attr, ci, value) + end + return +end + +function _set_substituted( + b::AbstractBridgeOptimizer, + ::MOI.ConstraintFunction, + ci::MOI.ConstraintIndex{F,S}, + value, +) where {F,S} + if F == MOI.VariableIndex + throw(MOI.SettingVariableIndexNotAllowed()) + elseif !(value isa F) + throw(ArgumentError("Invalid type when setting ConstraintFunction.")) + elseif is_bridged(b, ci) + MOI.throw_if_not_valid(b, ci) + call_in_context(MOI.set, b, ci, attr, value) + else + MOI.set(b.model, attr, ci, value) + end + return +end + function _set_substituted( b::AbstractBridgeOptimizer, attr::MOI.AbstractConstraintAttribute, diff --git a/src/Test/test_modification.jl b/src/Test/test_modification.jl index bd298942c6..fd8de72f3c 100644 --- a/src/Test/test_modification.jl +++ b/src/Test/test_modification.jl @@ -932,3 +932,29 @@ function setup_test( ) return end + +""" + test_modification_incorrect(model::MOI.ModelLike, config::Config) + +Test that constraint sets cannot be set for the wrong set type, and that +VariableIndex functions cannot be modified. +""" +function test_modification_incorrect(model::MOI.ModelLike, ::Config) + @requires MOI.supports_constraint( + model, + MOI.ScalarAffineFunction{Float64}, + MOI.EqualTo{Float64}, + ) + x = MOI.add_variable(model) + c = MOI.add_constraint( + model, + MOI.ScalarAffineFunction([MOI.ScalarAffineTerm(1.0, x)], 0.0), + MOI.EqualTo(1.0), + ) + @test_throws( + ArgumentError, + MOI.set(model, MOI.ConstraintSet(), c, MOI.LessThan(1.0)), + ) + @test_throws(ArgumentError, MOI.set(model, MOI.ConstraintFunction(), c, x),) + return +end diff --git a/src/Test/test_objective.jl b/src/Test/test_objective.jl index 1d02194b9c..87e7921f21 100644 --- a/src/Test/test_objective.jl +++ b/src/Test/test_objective.jl @@ -517,29 +517,3 @@ function test_objective_set_via_modify(model::MOI.ModelLike, config::Config) @test MOI.get(model, MOI.ListOfModelAttributesSet()) == [attr] return end - -""" - test_objective_incorrect_modifications(model::MOI.ModelLike, config::Config) - -Test that constraint sets cannot be set for the wrong set type, and that -VariableIndex functions cannot be modified. -""" -function test_objective_incorrect_modifications(model::MOI.ModelLike, ::Config) - @requires MOI.supports_constraint( - model, - MOI.ScalarAffineFunction{Float64}, - MOI.EqualTo{Float64}, - ) - x = MOI.add_variable(model) - c = MOI.add_constraint( - model, - MOI.ScalarAffineFunction([MOI.ScalarAffineTerm(1.0, x)], 0.0), - MOI.EqualTo(1.0), - ) - @test_throws( - ArgumentError, - MOI.set(model, MOI.ConstraintSet(), c, MOI.LessThan(1.0)), - ) - @test_throws(ArgumentError, MOI.set(model, MOI.ConstraintFunction(), c, x),) - return -end diff --git a/test/Bridges/bridge_optimizer.jl b/test/Bridges/bridge_optimizer.jl index 6e98f8e4ee..c2afbde99b 100644 --- a/test/Bridges/bridge_optimizer.jl +++ b/test/Bridges/bridge_optimizer.jl @@ -748,6 +748,18 @@ function test_recursive_model_objective(::Type{T} = Int) where {T} @test MOI.get(b, attr) ≈ x end +function test_invalid_modifications() + model = MOI.Bridges.full_bridge_optimizer( + MOI.Utilities.Model{Float64}(), + Float64, + ) + config = MOI.Test.Config() + MOI.Test.test_modification_set_function_single_variable(model, config) + MOI.empty!(model) + MOI.Test.test_modification_incorrect(model, config) + return +end + end # module TestBridgeOptimizer.runtests() From 0882d9487cb747a3b5c525cc5691e7b4f652146e Mon Sep 17 00:00:00 2001 From: Oscar Dowson Date: Tue, 14 Sep 2021 17:19:53 +1200 Subject: [PATCH 2/5] Update bridge_optimizer.jl --- src/Bridges/bridge_optimizer.jl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Bridges/bridge_optimizer.jl b/src/Bridges/bridge_optimizer.jl index 1c8a15149a..5aa08408e3 100644 --- a/src/Bridges/bridge_optimizer.jl +++ b/src/Bridges/bridge_optimizer.jl @@ -1246,7 +1246,7 @@ end function _set_substituted( b::AbstractBridgeOptimizer, - ::MOI.ConstraintSet, + attr::MOI.ConstraintSet, ci::MOI.ConstraintIndex{F,S}, value, ) where {F,S} @@ -1263,7 +1263,7 @@ end function _set_substituted( b::AbstractBridgeOptimizer, - ::MOI.ConstraintFunction, + attr::MOI.ConstraintFunction, ci::MOI.ConstraintIndex{F,S}, value, ) where {F,S} From a075cb783035d1bfae7ffed40f6d878940a3048c Mon Sep 17 00:00:00 2001 From: odow Date: Wed, 15 Sep 2021 10:19:06 +1200 Subject: [PATCH 3/5] Lift type dispatch to the MOI level --- src/Bridges/bridge_optimizer.jl | 62 +++++++++----------------- test/Bridges/Constraint/functionize.jl | 2 - 2 files changed, 21 insertions(+), 43 deletions(-) diff --git a/src/Bridges/bridge_optimizer.jl b/src/Bridges/bridge_optimizer.jl index 5aa08408e3..5ce560719e 100644 --- a/src/Bridges/bridge_optimizer.jl +++ b/src/Bridges/bridge_optimizer.jl @@ -1104,14 +1104,15 @@ function MOI.set( end # Constraint attributes + function MOI.set( b::AbstractBridgeOptimizer, attr::MOI.ConstraintFunction, ci::MOI.ConstraintIndex{F}, - func::F, + func, ) where {F} - if F == MOI.VariableIndex - throw(MOI.SettingVariableIndexNotAllowed()) + if typeof(func) != F + throw(ArgumentError("Invalid type when setting ConstraintFunction.")) end if Variable.has_bridges(Variable.bridges(b)) set = MOI.get(b, MOI.ConstraintSet(), ci) @@ -1146,8 +1147,11 @@ function MOI.set( b::AbstractBridgeOptimizer, attr::MOI.ConstraintSet, ci::MOI.ConstraintIndex{MOI.VariableIndex,S}, - value::S, + value, ) where {S<:MOI.AbstractScalarSet} + if typeof(value) != S + throw(ArgumentError("Invalid type when setting ConstraintSet.")) + end _set_substituted(b, attr, ci, value) return end @@ -1169,8 +1173,11 @@ function MOI.set( b::AbstractBridgeOptimizer, attr::MOI.ConstraintSet, ci::MOI.ConstraintIndex{<:MOI.AbstractScalarFunction,S}, - set::S, + set, ) where {S<:MOI.AbstractScalarSet} + if typeof(set) != S + throw(ArgumentError("Invalid type when setting ConstraintSet.")) + end if Variable.has_bridges(Variable.bridges(b)) func = MOI.get(b, MOI.ConstraintFunction(), ci) _, set = bridged_constraint_function(b, func, set) @@ -1244,42 +1251,6 @@ function MOI.supports( ) end -function _set_substituted( - b::AbstractBridgeOptimizer, - attr::MOI.ConstraintSet, - ci::MOI.ConstraintIndex{F,S}, - value, -) where {F,S} - if !(value isa S) - throw(ArgumentError("Invalid type when setting ConstraintSet.")) - elseif is_bridged(b, ci) - MOI.throw_if_not_valid(b, ci) - call_in_context(MOI.set, b, ci, attr, value) - else - MOI.set(b.model, attr, ci, value) - end - return -end - -function _set_substituted( - b::AbstractBridgeOptimizer, - attr::MOI.ConstraintFunction, - ci::MOI.ConstraintIndex{F,S}, - value, -) where {F,S} - if F == MOI.VariableIndex - throw(MOI.SettingVariableIndexNotAllowed()) - elseif !(value isa F) - throw(ArgumentError("Invalid type when setting ConstraintFunction.")) - elseif is_bridged(b, ci) - MOI.throw_if_not_valid(b, ci) - call_in_context(MOI.set, b, ci, attr, value) - else - MOI.set(b.model, attr, ci, value) - end - return -end - function _set_substituted( b::AbstractBridgeOptimizer, attr::MOI.AbstractConstraintAttribute, @@ -1305,6 +1276,15 @@ function MOI.set( return end +function MOI.set( + ::AbstractBridgeOptimizer, + ::MOI.ConstraintFunction, + ::MOI.ConstraintIndex{MOI.VariableIndex}, + ::MOI.VariableIndex, +) + return throw(MOI.SettingVariableIndexNotAllowed()) +end + ## Getting and Setting names function MOI.get( b::AbstractBridgeOptimizer, diff --git a/test/Bridges/Constraint/functionize.jl b/test/Bridges/Constraint/functionize.jl index 4db63d07ec..158b3651fa 100644 --- a/test/Bridges/Constraint/functionize.jl +++ b/test/Bridges/Constraint/functionize.jl @@ -27,8 +27,6 @@ function test_scalar_functionize() y = MOI.add_variable(bridged_mock) ci = MOI.add_constraint(bridged_mock, x, MOI.GreaterThan(0.0)) @test MOI.get(bridged_mock, MOI.ConstraintFunction(), ci) ≈ x - MOI.set(bridged_mock, MOI.ConstraintFunction(), ci, y) - @test MOI.get(bridged_mock, MOI.ConstraintFunction(), ci) ≈ y @test MOI.get(bridged_mock, MOI.ConstraintSet(), ci) == MOI.GreaterThan(0.0) MOI.set(bridged_mock, MOI.ConstraintSet(), ci, MOI.GreaterThan(1.0)) @test MOI.get(bridged_mock, MOI.ConstraintSet(), ci) == MOI.GreaterThan(1.0) From 64a960f42180472bfa311ecc83ea611d628a4f2a Mon Sep 17 00:00:00 2001 From: odow Date: Wed, 15 Sep 2021 15:56:02 +1200 Subject: [PATCH 4/5] Remove unneeded function --- src/Bridges/Constraint/functionize.jl | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/src/Bridges/Constraint/functionize.jl b/src/Bridges/Constraint/functionize.jl index 372c137250..526260d563 100644 --- a/src/Bridges/Constraint/functionize.jl +++ b/src/Bridges/Constraint/functionize.jl @@ -72,20 +72,6 @@ function MOI.delete(model::MOI.ModelLike, c::ScalarFunctionizeBridge) end # Constraints -function MOI.set( - model::MOI.ModelLike, - ::MOI.ConstraintFunction, - c::ScalarFunctionizeBridge{T}, - f::MOI.VariableIndex, -) where {T} - MOI.set( - model, - MOI.ConstraintFunction(), - c.constraint, - MOI.ScalarAffineFunction{T}(f), - ) - return -end function MOI.get( model::MOI.ModelLike, From 8c88d7c78de15e737ee59c249d9796e8c0e46f71 Mon Sep 17 00:00:00 2001 From: odow Date: Wed, 15 Sep 2021 18:43:18 +1200 Subject: [PATCH 5/5] More tests --- src/Test/test_modification.jl | 40 +++++++++++++++++++++++++++++--- test/Bridges/bridge_optimizer.jl | 2 ++ 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/src/Test/test_modification.jl b/src/Test/test_modification.jl index fd8de72f3c..21bb1150c2 100644 --- a/src/Test/test_modification.jl +++ b/src/Test/test_modification.jl @@ -936,8 +936,7 @@ end """ test_modification_incorrect(model::MOI.ModelLike, config::Config) -Test that constraint sets cannot be set for the wrong set type, and that -VariableIndex functions cannot be modified. +Test that constraint sets and functions cannot be set for the wrong type. """ function test_modification_incorrect(model::MOI.ModelLike, ::Config) @requires MOI.supports_constraint( @@ -955,6 +954,41 @@ function test_modification_incorrect(model::MOI.ModelLike, ::Config) ArgumentError, MOI.set(model, MOI.ConstraintSet(), c, MOI.LessThan(1.0)), ) - @test_throws(ArgumentError, MOI.set(model, MOI.ConstraintFunction(), c, x),) + @test_throws(ArgumentError, MOI.set(model, MOI.ConstraintFunction(), c, x)) + return +end + +""" + test_modification_incorrect_VariableIndex( + model::MOI.ModelLike, + config::Config, + ) + +Test that constraint sets cannot be set for the wrong set type, and that +VariableIndex functions cannot be modified. +""" +function test_modification_incorrect_VariableIndex( + model::MOI.ModelLike, + ::Config{T}, +) where {T} + @requires MOI.supports_constraint(model, MOI.VariableIndex, MOI.LessThan{T}) + @requires( + MOI.supports_constraint(model, MOI.VariableIndex, MOI.GreaterThan{T}), + ) + x = MOI.add_variable(model) + c = MOI.add_constraint(model, x, MOI.GreaterThan(zero(T))) + @test_throws( + ArgumentError, + MOI.set(model, MOI.ConstraintSet(), c, MOI.LessThan(one(T))), + ) + @test_throws( + ArgumentError, + MOI.set(model, MOI.ConstraintFunction(), c, one(T) * x), + ) + y = MOI.add_variable(model) + @test_throws( + MOI.SettingVariableIndexNotAllowed(), + MOI.set(model, MOI.ConstraintFunction(), c, y), + ) return end diff --git a/test/Bridges/bridge_optimizer.jl b/test/Bridges/bridge_optimizer.jl index c2afbde99b..103158b8a9 100644 --- a/test/Bridges/bridge_optimizer.jl +++ b/test/Bridges/bridge_optimizer.jl @@ -757,6 +757,8 @@ function test_invalid_modifications() MOI.Test.test_modification_set_function_single_variable(model, config) MOI.empty!(model) MOI.Test.test_modification_incorrect(model, config) + MOI.empty!(model) + MOI.Test.test_modification_incorrect_VariableIndex(model, config) return end