Merge pull request #396 from JuliaDiff/ox/inplaceorder

Change argument order on InplaceableThunk and fix deprecated tests

Author: oxinabox

Project.toml

@@ -1,6 +1,6 @@
 name = "ChainRulesCore"
 uuid = "d360d2e6-b24c-11e9-a2a3-2a2ae2dbcce4"
-version = "0.10.11"
+version = "0.10.12"
 
 [deps]
 Compat = "34da2185-b29b-5c13-b0c7-acf172513d20"

src/deprecated.jl

@@ -54,3 +54,6 @@ for T in (:Thunk, :InplaceableThunk)
         return unthunk(x)
     end    
 end
+
+
+Base.@deprecate InplaceableThunk(t::Thunk, add!) InplaceableThunk(add!, t)

src/differentials/thunks.jl

@@ -197,7 +197,7 @@ Base.convert(::Type{<:Thunk}, a::AbstractZero) = @thunk(a)
 
 
 """
-    InplaceableThunk(val::Thunk, add!::Function)
+    InplaceableThunk(add!::Function, val::Thunk)
 
 A wrapper for a `Thunk`, that allows it to define an inplace `add!` function.
 
@@ -209,8 +209,8 @@ Most operations on an `InplaceableThunk` treat it just like a normal `Thunk`;
 and destroy its inplacability.
 """
 struct InplaceableThunk{T<:Thunk,F} <: AbstractThunk
-    val::T
     add!::F
+    val::T
 end
 
 unthunk(x::InplaceableThunk) = unthunk(x.val)

test/accumulation.jl

@@ -89,7 +89,7 @@
 
         @testset "AbstractThunk $(typeof(thunk))" for thunk in (
             @thunk(-1.0*ones(2, 2)),
-            InplaceableThunk(@thunk(-1.0*ones(2, 2)), x -> x .-= ones(2, 2)),
+            InplaceableThunk(x -> x .-= ones(2, 2), @thunk(-1.0*ones(2, 2))),
         )
             @testset "in place" begin
                 accumuland = [1.0 2.0; 3.0 4.0]
@@ -109,10 +109,10 @@
         end
 
         @testset "not actually inplace but said it was" begin
-            ithunk = InplaceableThunk(
-                @thunk(@assert false),  # this should never be used in this test
-                x -> 77*ones(2, 2)  # not actually inplace (also wrong)
-            )
+            # thunk should never be used in this test
+            ithunk = InplaceableThunk(@thunk(@assert false)) do x
+                77*ones(2, 2)  # not actually inplace (also wrong)
+            end
             accumuland = ones(2, 2)
             @assert ChainRulesCore.debug_mode() == false
             # without debug being enabled should return the result, not error
@@ -127,7 +127,7 @@
 
     @testset "showerror BadInplaceException" begin
         BadInplaceException = ChainRulesCore.BadInplaceException
-        ithunk = InplaceableThunk(@thunk(@assert false), x̄->nothing)
+        ithunk = InplaceableThunk(x̄->nothing, @thunk(@assert false))
         msg = sprint(showerror, BadInplaceException(ithunk, [22], [23]))
         @test occursin("22", msg)
 

test/deprecated.jl

@@ -1,30 +1,39 @@
 @testset "NO_FIELDS" begin
-    @test (@test_deprecated NO_FIELDS) isa NoTangent
+    # Following doesn't work because of some deprecate_binding weirdness with not printing
+    # @test (@test_deprecated NO_FIELDS) isa NoTangent
+    # So just test it gives the old behavour
+    @test NO_FIELDS isa NoTangent
 end
 
 @testset "extern" begin
-    @test extern(@thunk(3)) == 3
-    @test extern(@thunk(@thunk(3))) == 3
+    @test (@test_deprecated extern(@thunk(3))) == 3
+    @test (@test_deprecated extern(@thunk(@thunk(3)))) == 3
 
-    @test extern(Tangent{Foo}(x=2.0)) == (;x=2.0)
-    @test extern(Tangent{Tuple{Float64,}}(2.0)) == (2.0,)
-    @test extern(Tangent{Dict}(Dict(4 => 3))) == Dict(4 => 3)
+    @test (@test_deprecated extern(Tangent{Foo}(x=2.0))) == (;x=2.0)
+    @test (@test_deprecated extern(Tangent{Tuple{Float64,}}(2.0))) == (2.0,)
+    @test (@test_deprecated extern(Tangent{Dict}(Dict(4 => 3)))) == Dict(4 => 3)
 
     # with differentials on the inside
-    @test extern(Tangent{Foo}(x=@thunk(0+2.0))) == (;x=2.0)
-    @test extern(Tangent{Tuple{Float64,}}(@thunk(0+2.0))) == (2.0,)
-    @test extern(Tangent{Dict}(Dict(4 => @thunk(3)))) == Dict(4 => 3)
+    @test (@test_deprecated extern(Tangent{Foo}(x=@thunk(0+2.0)))) == (;x=2.0)
+    @test (@test_deprecated extern(Tangent{Tuple{Float64,}}(@thunk(0+2.0)))) == (2.0,)
+    @test (@test_deprecated extern(Tangent{Dict}(Dict(4 => @thunk(3))))) == Dict(4 => 3)
 
     z = ZeroTangent()
-    @test extern(z) === false
+    @test (@test_deprecated extern(z)) === false
+    
+    # @test_throws doesn't play nice with `@test_deprecated` so have to be loud
     dne = NoTangent()
     @test_throws Exception extern(dne)
-    E = ChainRulesCore.NotImplementedException
-    @test_throws E extern(ni)
+    ni = @not_implemented("no")
+    @test_throws ChainRulesCore.NotImplementedException extern(ni)
 end
 
 
 @testset "Deprecated: calling thunks should call inner function" begin
-    @test_deprecated (@thunk(3))() == 3
-    @test_deprecated (@thunk(@thunk(3)))() isa Thunk
+    @test (@test_deprecated (@thunk(3))()) == 3
+    @test (@test_deprecated (@thunk(@thunk(3)))()) isa Thunk
 end
+
+@testset "Deprecated: Inplacable Thunk argument order" begin
+    @test (@test_deprecated InplaceableThunk(@thunk([1]), x->x.+=1)) isa InplaceableThunk
+end

test/differentials/thunks.jl

@@ -4,9 +4,9 @@
     @test @thunk(3) isa Thunk
 
     @testset "==" begin
-        @test @thunk(3.2) == InplaceableThunk(@thunk(3.2), x -> x + 3.2)
+        @test @thunk(3.2) == InplaceableThunk(x -> x + 3.2, @thunk(3.2))
         @test @thunk(3.2) == 3.2
-        @test 3.2 == InplaceableThunk(@thunk(3.2), x -> x + 3.2)
+        @test 3.2 == InplaceableThunk(x -> x + 3.2, @thunk(3.2))
     end
 
     @testset "iterate" begin

test/runtests.jl

@@ -21,4 +21,6 @@ using Test
     include("rules.jl")
     include("rule_definition_tools.jl")
     include("config.jl")
+
+    include("deprecated.jl")
 end