From c53e704ddafd6f0b20ae4db807e2137e29b8dcee Mon Sep 17 00:00:00 2001 From: odow Date: Fri, 16 Feb 2024 14:11:22 +1300 Subject: [PATCH 1/3] Throw error if Containers.SparseAxisArray used in constraint function --- src/macros/@constraint.jl | 45 +++++++++++++++++++++++++++++++++++++++ test/test_macros.jl | 15 +++++++++++++ 2 files changed, 60 insertions(+) diff --git a/src/macros/@constraint.jl b/src/macros/@constraint.jl index bc0feeb03a8..ab0a2844f1e 100644 --- a/src/macros/@constraint.jl +++ b/src/macros/@constraint.jl @@ -847,6 +847,51 @@ function build_constraint( return VectorConstraint(x, set) end +function build_constraint( + error_fn::Function, + ::Containers.SparseAxisArray{<:Union{Number,AbstractJuMPScalar},1}, + ::MOI.AbstractVectorSet, +) + return error_fn( + """ + Building a constraint in which the function is a `Containers.SparseAxisArray`. + + ## Work-around + + Convert `x` into a vector by explicitly providing an ordered list of + indices. For example, instead of: + + ```julia + A = [[1, 2, 10], [2, 3, 30]] + model = Model() + @variable(model, x[i in 1:2, j in A[i]]) + @constraint(model, x[1, :] in SecondOrderCone()) + ``` + + do: + + ```julia + A = [[1, 2, 10], [2, 3, 30]] + model = Model() + @variable(model, x[i in 1:2, j in A[i]]) + y = [x[1, j] for j in A[1]]::Vector{VariableRef} + @constraint(model, y in SecondOrderCone()) + ``` + + ## Explanation for breaking change in JuMP v1.21 + + Prior to JuMP v1.20, this constraint would have been allowed. It is now + an error because of the high risk of incorrect usage. + + The backend data structure of a `Containers.SparseAxisArray` is a + `Dict`. JuMP used to create the constraint with + `[x[k] for k in eachindex(x)]`, however, the iteration order of + `eachindex` is undefined. This can silently cause incorrect models to be + built if the set of the constraint depends on the order of the elements. + """ + ) +end + function build_constraint( error_fn::Function, x::AbstractArray, diff --git a/test/test_macros.jl b/test/test_macros.jl index 03652a0f679..1c2fee18389 100644 --- a/test/test_macros.jl +++ b/test/test_macros.jl @@ -2358,4 +2358,19 @@ function test_op_or_short_circuit() return end +function test_sparseaxisarray_constraint_zeros() + A = [[1, 2, 10], [2, 3, 30]] + model = Model() + @variable(model, x[i in 1:2, j in A[i]]) + @test_throws_runtime( + ErrorException, + @constraint(model, x[1, :] == 0), + ) + @test_throws_runtime( + ErrorException, + @constraint(model, x[1, :] in SecondOrderCone()), + ) + return +end + end # module From aae7f86bbd15b755d85fcae1f9cc087e4a83a547 Mon Sep 17 00:00:00 2001 From: odow Date: Fri, 16 Feb 2024 14:25:40 +1300 Subject: [PATCH 2/3] Fix ambiguity --- src/macros/@constraint.jl | 94 +++++++++++++++++++++++---------------- test/test_macros.jl | 15 ++++++- 2 files changed, 69 insertions(+), 40 deletions(-) diff --git a/src/macros/@constraint.jl b/src/macros/@constraint.jl index ab0a2844f1e..740b57ecf81 100644 --- a/src/macros/@constraint.jl +++ b/src/macros/@constraint.jl @@ -847,49 +847,51 @@ function build_constraint( return VectorConstraint(x, set) end +function _build_sparse_axis_array_error_msg() + return """ + Building a constraint in which the function is a `Containers.SparseAxisArray`. + + ## Work-around + + Convert `x` into a vector by explicitly providing an ordered list of + indices. For example, instead of: + + ```julia + A = [[1, 2, 10], [2, 3, 30]] + model = Model() + @variable(model, x[i in 1:2, j in A[i]]) + @constraint(model, x[1, :] in SecondOrderCone()) + ``` + + do: + + ```julia + A = [[1, 2, 10], [2, 3, 30]] + model = Model() + @variable(model, x[i in 1:2, j in A[i]]) + y = [x[1, j] for j in A[1]]::Vector{VariableRef} + @constraint(model, y in SecondOrderCone()) + ``` + + ## Explanation for breaking change in JuMP v1.21 + + Prior to JuMP v1.20, this constraint would have been allowed. It is now + an error because of the high risk of incorrect usage. + + The backend data structure of a `Containers.SparseAxisArray` is a + `Dict`. JuMP used to create the constraint with + `[x[k] for k in eachindex(x)]`, however, the iteration order of + `eachindex` is undefined. This can silently cause incorrect models to be + built if the set of the constraint depends on the order of the elements. + """ +end + function build_constraint( error_fn::Function, ::Containers.SparseAxisArray{<:Union{Number,AbstractJuMPScalar},1}, ::MOI.AbstractVectorSet, ) - return error_fn( - """ - Building a constraint in which the function is a `Containers.SparseAxisArray`. - - ## Work-around - - Convert `x` into a vector by explicitly providing an ordered list of - indices. For example, instead of: - - ```julia - A = [[1, 2, 10], [2, 3, 30]] - model = Model() - @variable(model, x[i in 1:2, j in A[i]]) - @constraint(model, x[1, :] in SecondOrderCone()) - ``` - - do: - - ```julia - A = [[1, 2, 10], [2, 3, 30]] - model = Model() - @variable(model, x[i in 1:2, j in A[i]]) - y = [x[1, j] for j in A[1]]::Vector{VariableRef} - @constraint(model, y in SecondOrderCone()) - ``` - - ## Explanation for breaking change in JuMP v1.21 - - Prior to JuMP v1.20, this constraint would have been allowed. It is now - an error because of the high risk of incorrect usage. - - The backend data structure of a `Containers.SparseAxisArray` is a - `Dict`. JuMP used to create the constraint with - `[x[k] for k in eachindex(x)]`, however, the iteration order of - `eachindex` is undefined. This can silently cause incorrect models to be - built if the set of the constraint depends on the order of the elements. - """ - ) + return error_fn(_build_sparse_axis_array_error_msg()) end function build_constraint( @@ -1008,3 +1010,19 @@ function build_constraint( MOI.SOS2{value_type(variable_ref_type(T))}(set.weights), ) end + +function build_constraint( + error_fn::Function, + ::Containers.SparseAxisArray{<:Union{Number,AbstractJuMPScalar},1}, + ::MOI.SOS1, +) + return error_fn(_build_sparse_axis_array_error_msg()) +end + +function build_constraint( + error_fn::Function, + ::Containers.SparseAxisArray{<:Union{Number,AbstractJuMPScalar},1}, + ::MOI.SOS2, +) + return error_fn(_build_sparse_axis_array_error_msg()) +end diff --git a/test/test_macros.jl b/test/test_macros.jl index 1c2fee18389..a9be260c2dc 100644 --- a/test/test_macros.jl +++ b/test/test_macros.jl @@ -2362,14 +2362,25 @@ function test_sparseaxisarray_constraint_zeros() A = [[1, 2, 10], [2, 3, 30]] model = Model() @variable(model, x[i in 1:2, j in A[i]]) + msg = JuMP._build_sparse_axis_array_error_msg() @test_throws_runtime( - ErrorException, + ErrorException("In `@constraint(model, x[1, :] == 0)`: $msg"), @constraint(model, x[1, :] == 0), ) @test_throws_runtime( - ErrorException, + ErrorException( + "In `@constraint(model, x[1, :] in SecondOrderCone())`: $msg", + ), @constraint(model, x[1, :] in SecondOrderCone()), ) + @test_throws_runtime( + ErrorException("In `@constraint(model, x[1, :] in SOS1())`: $msg"), + @constraint(model, x[1, :] in SOS1()), + ) + @test_throws_runtime( + ErrorException("In @constraint(model, x[1, :] in SOS2())`: $msg"), + @constraint(model, x[1, :] in SOS2()), + ) return end From ae03657c6268adb258e2e2b1b30e4a991d38f4b4 Mon Sep 17 00:00:00 2001 From: Oscar Dowson Date: Fri, 16 Feb 2024 14:44:54 +1300 Subject: [PATCH 3/3] Update test/test_macros.jl --- test/test_macros.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_macros.jl b/test/test_macros.jl index a9be260c2dc..a7d3c02027c 100644 --- a/test/test_macros.jl +++ b/test/test_macros.jl @@ -2378,7 +2378,7 @@ function test_sparseaxisarray_constraint_zeros() @constraint(model, x[1, :] in SOS1()), ) @test_throws_runtime( - ErrorException("In @constraint(model, x[1, :] in SOS2())`: $msg"), + ErrorException("In `@constraint(model, x[1, :] in SOS2())`: $msg"), @constraint(model, x[1, :] in SOS2()), ) return