diff --git a/src/Bridges/debug.jl b/src/Bridges/debug.jl index 09dc90c337..03972e4b76 100644 --- a/src/Bridges/debug.jl +++ b/src/Bridges/debug.jl @@ -664,7 +664,7 @@ function print_active_bridges( # The constraint can be both variable bridged and constraint # bridged. Which is cheaper? v_node = b.variable_node[(S,)] - if bridging_cost(b.graph, v_node) <= bridging_cost(b.graph, c_node) + if MOI.Bridges.is_variable_edge_best(b.graph, v_node) return print_active_bridges(io, b, S, offset) end else diff --git a/src/Bridges/graph.jl b/src/Bridges/graph.jl index 93db56160e..081ae25c6a 100644 --- a/src/Bridges/graph.jl +++ b/src/Bridges/graph.jl @@ -330,7 +330,22 @@ constrained on creation, or as a free variable followed by a constraint. """ function is_variable_edge_best(graph::Graph, node::VariableNode) _compute_bellman_ford(graph) - return graph.variable_dist[node.index] == _dist(graph, node) + # This is the cost of adding a constrained variable + dist_as_variable = graph.variable_dist[node.index] + # This is the cost of adding the constraint, if we were to add it. + dist_as_constraint = INFINITY + # If free variables are bridged but the functionize bridge was not + # added, constraint_node is `ConstraintNode(INVALID_NODE_INDEX)`. + constraint_node = graph.variable_constraint_node[node.index] + if constraint_node.index != INVALID_NODE_INDEX + dist_as_constraint = _dist(graph, constraint_node) + if dist_as_constraint != INFINITY + dist_as_constraint += graph.variable_constraint_cost[node.index] + end + end + # We should prefer the variable bridge ONLY if the cost is strictly less + # than bridging via constraints. + return dist_as_variable < dist_as_constraint end function _updated_dist( diff --git a/src/Bridges/lazy_bridge_optimizer.jl b/src/Bridges/lazy_bridge_optimizer.jl index 37255c56b9..d89e2073a7 100644 --- a/src/Bridges/lazy_bridge_optimizer.jl +++ b/src/Bridges/lazy_bridge_optimizer.jl @@ -293,8 +293,17 @@ function node( ) end else - # We add `+1` as we treat constrained variables as constraints. - set_variable_constraint_node(b.graph, variable_node, node(b, F, S), 1) + # Add a node to the graph so we can convert a constrained variable into + # a free variable plus a constraint. In MOI v1.50 and earlier, the cost + # associated with this node was +1. However, practice has shown that we + # want to encourage constraint bridges over variable bridges, and + # penalizing the switch from constrained variable to constraint with an + # additional +1 meant we often used a bridge like Variable.Vectorize. + # With +0 here, it's now better to use Constraint.Vectorize. + # + # For more details, see: + # https://github.com/jump-dev/ParametricOptInterface.jl/issues/201 + set_variable_constraint_node(b.graph, variable_node, node(b, F, S), +0) end for (i, BT) in enumerate(b.variable_bridge_types) if Variable.supports_constrained_variable(BT, S) diff --git a/test/Bridges/General/test_bridge_optimizer.jl b/test/Bridges/General/test_bridge_optimizer.jl index 19de4d87c2..5e32ad9e6c 100644 --- a/test/Bridges/General/test_bridge_optimizer.jl +++ b/test/Bridges/General/test_bridge_optimizer.jl @@ -1240,6 +1240,16 @@ function test_issue_2452_multiple_variable_bridges() MOI.add_constraint(src, x, MOI.LessThan(1.0)) c = MOI.add_constraint(src, 2.0 * x, MOI.EqualTo(3.0)) dest = MOI.instantiate(Model2452{Float64}; with_bridge_type = Float64) + # Remove constraint bridges so that we do + # x in LessThan -> [y] in Nonpositives, y:=x-1, [z] in Nonnegatives, z:=-y + MOI.Bridges.remove_bridge( + dest, + MOI.Bridges.Constraint.LessToGreaterBridge{Float64}, + ) + MOI.Bridges.remove_bridge( + dest, + MOI.Bridges.Constraint.NonposToNonnegBridge{Float64}, + ) index_map = MOI.copy_to(dest, src) set = MOI.get(dest, MOI.ConstraintSet(), index_map[c]) @test set == MOI.EqualTo(3.0) @@ -1264,13 +1274,11 @@ function test_2452() @test MOI.get(dest, MOI.ConstraintSet(), index_map[c]) == set @test ≈( MOI.get(dest.model, MOI.ConstraintFunction(), ci), - MOI.Utilities.operate(vcat, Float64, -1.0 + 2.0 * y), + MOI.Utilities.operate(vcat, Float64, -3.0 + 2.0 * y), ) @test MOI.get(dest.model, MOI.ConstraintSet(), ci) == MOI.Zeros(1) - @test_throws( - MOI.SetAttributeNotAllowed, - MOI.set(dest, MOI.ConstraintSet(), index_map[c], set), - ) + MOI.set(dest, MOI.ConstraintSet(), index_map[c], set) + @test MOI.get(dest.model, MOI.ConstraintSet(), ci) == MOI.Zeros(1) return end diff --git a/test/Bridges/General/test_lazy_bridge_optimizer.jl b/test/Bridges/General/test_lazy_bridge_optimizer.jl index 2d28d77a45..3ebb38d7fc 100644 --- a/test/Bridges/General/test_lazy_bridge_optimizer.jl +++ b/test/Bridges/General/test_lazy_bridge_optimizer.jl @@ -1306,6 +1306,11 @@ MOI.Utilities.@model( function _test_constrained_variables_in_RSOC(T) bridged = MOI.Bridges.full_bridge_optimizer(NoRSOCModel{T}(), T) + # Remove the constraint bridge to force via Variable bridges. + MOI.Bridges.remove_bridge( + bridged, + MOI.Bridges.Constraint.RSOCtoSOCBridge{T}, + ) @test MOI.supports_constraint( bridged, MOI.VectorOfVariables, diff --git a/test/Utilities/test_copy.jl b/test/Utilities/test_copy.jl index ef3c8671a1..ff8181e498 100644 --- a/test/Utilities/test_copy.jl +++ b/test/Utilities/test_copy.jl @@ -482,7 +482,7 @@ function test_create_variables_using_supports_add_constrained_variable() Type[MOI.Nonpositives, MOI.Zeros, MOI.Nonnegatives] @test MOI.supports_add_constrained_variables(bridged_dest, MOI.Nonnegatives) @test MOI.get(bridged_dest, MOI.VariableBridgingCost{MOI.Nonnegatives}()) == - 2.0 + 1.0 @test MOI.supports_constraint( bridged_dest, MOI.VectorOfVariables, @@ -546,7 +546,7 @@ function test_create_variables_using_supports_add_constrained_variable() @test MOI.get( bridged_dest, MOI.VariableBridgingCost{MOI.GreaterThan{Float64}}(), - ) == 1.0 + ) == 0.0 @test MOI.get( bridged_dest, MOI.ConstraintBridgingCost{MOI.VariableIndex,MOI.GreaterThan{Float64}}(), @@ -560,7 +560,7 @@ function test_create_variables_using_supports_add_constrained_variable() @test MOI.get( bridged_dest, MOI.VariableBridgingCost{MOI.LessThan{Float64}}(), - ) == 1.0 + ) == 0.0 @test MOI.get( bridged_dest, MOI.ConstraintBridgingCost{MOI.VariableIndex,MOI.LessThan{Float64}}(),