From ee43d21fba289f46c581a768d6929b53353353b8 Mon Sep 17 00:00:00 2001 From: odow Date: Thu, 20 Dec 2018 14:19:00 -0600 Subject: [PATCH 1/5] Fix bug handling Adjoints in macros --- src/parse_expr.jl | 6 ++++++ test/macros.jl | 11 +++++++++++ 2 files changed, 17 insertions(+) diff --git a/src/parse_expr.jl b/src/parse_expr.jl index cdeeb8ce29f..35c231dc10a 100644 --- a/src/parse_expr.jl +++ b/src/parse_expr.jl @@ -285,6 +285,12 @@ function destructive_add!(ex::AbstractArray{<:GenericAffExpr}, c::Number, return result end +# For cases such as x' * x, broadcasting the ex .+ will cause the Adjoint to be +# collected into an Array{T, 2}. The easiest way to work around this is to undo +# the adjoint, perform the operation, and then re-apply the adjoint. +function destructive_add!(ex::Number, c::Number, x::Adjoint{T, Vector{T}}) where {T} + return (ex .+ c * x')' +end destructive_add!(ex, c, x) = ex .+ c * x diff --git a/test/macros.jl b/test/macros.jl index 23bf1aaa2a7..1adc58d4141 100644 --- a/test/macros.jl +++ b/test/macros.jl @@ -270,6 +270,17 @@ end "DenseAxisArray, or SparseAxisArray.") @test_throws exception @variable(model, x[1:3], container=Oops) end + + @testset "Adjoints" begin + model = Model() + @variable(model, x[1:2]) + obj = @objective(model, Min, x' * x) + @test obj == sum(x.^2) + cref = @constraint(model, x' * x <= 1) + c = JuMP.constraint_object(cref) + @test JuMP.isequal_canonical(c.func, sum(x.^2)) + @test c.set == MOI.LessThan(1.0) + end end @testset "Macros for JuMPExtension.MyModel" begin From 7168e7f49f8abf8600aa29657a49305df8f2346c Mon Sep 17 00:00:00 2001 From: odow Date: Fri, 21 Dec 2018 10:37:35 -0600 Subject: [PATCH 2/5] Explicitly call broadcast in destructive_add! instead of relying on lowering. This is a work-around for a weird feature of Julia that doesn't propagate adjoints properly through `.` broadcasts. --- src/parse_expr.jl | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/parse_expr.jl b/src/parse_expr.jl index 35c231dc10a..05239516b78 100644 --- a/src/parse_expr.jl +++ b/src/parse_expr.jl @@ -285,15 +285,13 @@ function destructive_add!(ex::AbstractArray{<:GenericAffExpr}, c::Number, return result end -# For cases such as x' * x, broadcasting the ex .+ will cause the Adjoint to be -# collected into an Array{T, 2}. The easiest way to work around this is to undo -# the adjoint, perform the operation, and then re-apply the adjoint. -function destructive_add!(ex::Number, c::Number, x::Adjoint{T, Vector{T}}) where {T} - return (ex .+ c * x')' +# For some reason, the broadcast syntax `ex .+ c * x` fails if `x` is an +# `Adjoint`. But if we explicitly call `broadcast` it seems to work. +# See JuMP PR #1698 for more discussion. +function destructive_add!(ex, c, x) + return Broadcast.materialize(Broadcast.broadcast(+, ex, c * x)) end -destructive_add!(ex, c, x) = ex .+ c * x - destructive_add_with_reorder!(ex, arg) = destructive_add!(ex, 1.0, arg) # Special case because "Val{false}()" is used as the default empty expression. destructive_add_with_reorder!(ex::Val{false}, arg) = copy(arg) From 4029d92471abe87630c1899919f00cb10b202054 Mon Sep 17 00:00:00 2001 From: Oscar Dowson Date: Fri, 21 Dec 2018 11:37:51 -0600 Subject: [PATCH 3/5] Update parse_expr.jl --- src/parse_expr.jl | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/parse_expr.jl b/src/parse_expr.jl index 05239516b78..0e3650492f1 100644 --- a/src/parse_expr.jl +++ b/src/parse_expr.jl @@ -288,9 +288,7 @@ end # For some reason, the broadcast syntax `ex .+ c * x` fails if `x` is an # `Adjoint`. But if we explicitly call `broadcast` it seems to work. # See JuMP PR #1698 for more discussion. -function destructive_add!(ex, c, x) - return Broadcast.materialize(Broadcast.broadcast(+, ex, c * x)) -end +destructive_add!(ex, c, x) = broadcast(+, ex, c * x) destructive_add_with_reorder!(ex, arg) = destructive_add!(ex, 1.0, arg) # Special case because "Val{false}()" is used as the default empty expression. From 39a39c28d524ff8244b01b74c0d945e5e0b0e0c8 Mon Sep 17 00:00:00 2001 From: odow Date: Sun, 23 Dec 2018 17:14:43 -0600 Subject: [PATCH 4/5] Make tests catch adjoint bug properly --- test/macros.jl | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/test/macros.jl b/test/macros.jl index 1adc58d4141..6869087bed2 100644 --- a/test/macros.jl +++ b/test/macros.jl @@ -274,12 +274,16 @@ end @testset "Adjoints" begin model = Model() @variable(model, x[1:2]) - obj = @objective(model, Min, x' * x) - @test obj == sum(x.^2) - cref = @constraint(model, x' * x <= 1) + Q = [1.0 0.0; 0.0 1.0] + obj = @objective(model, Min, x' * Q * x) + @test JuMP.isequal_canonical(obj, sum(x.^2)) + cref = @constraint(model, x' * Q * x <= 1) c = JuMP.constraint_object(cref) @test JuMP.isequal_canonical(c.func, sum(x.^2)) @test c.set == MOI.LessThan(1.0) + @test JuMP.isequal_canonical( + JuMP.destructive_add!(0.0, x', Q), (1.0 .* x)' + ) end end From 40a258090f3a73564b198b069d06f74f2d53b464 Mon Sep 17 00:00:00 2001 From: Oscar Dowson Date: Wed, 26 Dec 2018 17:34:35 -0600 Subject: [PATCH 5/5] Update macros.jl --- test/macros.jl | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/test/macros.jl b/test/macros.jl index 6869087bed2..694ffcc719c 100644 --- a/test/macros.jl +++ b/test/macros.jl @@ -274,15 +274,14 @@ end @testset "Adjoints" begin model = Model() @variable(model, x[1:2]) - Q = [1.0 0.0; 0.0 1.0] - obj = @objective(model, Min, x' * Q * x) - @test JuMP.isequal_canonical(obj, sum(x.^2)) - cref = @constraint(model, x' * Q * x <= 1) + obj = @objective(model, Min, x' * ones(2, 2) * x) + @test JuMP.isequal_canonical(obj, x[1]^2 + 2 * x[1] * x[2] + x[2]^2) + cref = @constraint(model, x' * ones(2, 2) * x <= 1) c = JuMP.constraint_object(cref) - @test JuMP.isequal_canonical(c.func, sum(x.^2)) + @test JuMP.isequal_canonical(c.func, x[1]^2 + 2 * x[1] * x[2] + x[2]^2) @test c.set == MOI.LessThan(1.0) @test JuMP.isequal_canonical( - JuMP.destructive_add!(0.0, x', Q), (1.0 .* x)' + JuMP.destructive_add!(0.0, x', ones(2, 2)), x' * ones(2, 2) ) end end