Merge pull request #115 from denizyuret/dy/sparsebugs

Fixing #114 Sparse bugs

NEWS.md

@@ -1,5 +1,10 @@
+AutoGrad v1.1.7 Release Notes
+=============================
+
+
 AutoGrad v1.1.6 Release Notes
 =============================
+1dcce22 2019-09-30
 
 * Gradients returned can now be of type AutoGrad.Sparse to make large lookup parameters more efficient.
 * Refactoring: UngetIndex -> Sparse, sum_outgrad -> addto!, Tape.list back to normal order.

Project.toml

@@ -1,7 +1,7 @@
 name = "AutoGrad"
 uuid = "6710c13c-97f1-543f-91c5-74e8f7d95b35"
 authors = ["Deniz Yuret <[email protected]>"]
-version = "1.1.6"
+version = "1.1.7"
 
 [deps]
 Libdl = "8f399da3-3557-5675-b5ff-fb832c97cbdb"

src/addto.jl

@@ -37,10 +37,12 @@ matches(::AbstractDict,::AbstractDict)=true
 matches(a::Tuple,b::Tuple)=(length(a)===length(b))
 matches(a::AbstractArray,b::AbstractArray)=(size(a)==size(b))
 
-## If both accumulator and newval are sparse, merge:
+## If both accumulator and newval are sparse, merge, modifying first arg. Use + if you do not want to modify:
 function addto!(a::Sparse, b::Sparse)
     @assert matches(a.container, b.container) "$(summary.((a.container, b.container)))"
-    Sparse(a.container, [ a.values; b.values ], [ a.indices; b.indices ])
+    append!(a.values, b.values)
+    append!(a.indices, b.indices)
+    return a
 end
 
 ## If sparse is the accumulator, reverse:

src/sparse.jl

@@ -47,8 +47,8 @@ end
 
 # These are used in Knet/src/update.jl:
 import LinearAlgebra: axpy!, norm, lmul!
-axpy!(a, x::Sparse, y::AbstractArray) = addto!(y, a*x)
-lmul!(a, x::Sparse{T,N}) where {T,N} = Sparse{T,N}(x.container, [ a*v for v in x.values ], x.indices)
+axpy!(a::Number, x::Sparse, y::AbstractArray) = addto!(y, a*x)
+lmul!(a::Number, x::Sparse{T,N}) where {T,N} = Sparse{T,N}(x.container, [ a*v for v in x.values ], x.indices)
 
 # This does not give the correct result when there are repeated indices, but should be good enough for gclip
 norm(x::Sparse) = sqrt(sum(abs2, norm(v) for v in x.values))
@@ -76,6 +76,11 @@ broadcasted(::typeof(/), s::Sparse, n::Number) = Sparse(s.container, [ v./n for
 -(s::Sparse, a::AbstractArray) = addto!(-a, s)
 -(s::Sparse) = -1*s
 
+# Issue #114: we may need to add multiple gradients
+function +(a::Sparse, b::Sparse)
+    @assert matches(a.container, b.container) "$(summary.((a.container, b.container)))"
+    Sparse(a.container, [ a.values; b.values ], [ a.indices; b.indices ])
+end
 
 # Do we need these?
 # sum(b::Sparse)=sum(sum(v) for v in b.values)

test/runtests.jl

@@ -15,6 +15,7 @@ using Random; Random.seed!(1)
 #TODO include("params.jl")
 @time include("rosenbrock.jl")
 #TODO include("show.jl")
+@time include("sparse.jl")
 @time include("specialfunctions.jl")
 @time include("statistics.jl")
 #TODO include("unbroadcast.jl")

test/sparse.jl

@@ -0,0 +1,25 @@
+include("header.jl")
+
+@testset "sparse" begin
+
+    # Issue #114 (a): plus for Sparse.
+    using AutoGrad: Sparse, full, addto!
+    a = Sparse(zeros(3,4), [ [1.,1.], [1.,1.], 1., 1. ], [ ([1,2],), (3:4,), (2,2), (1,) ])
+    b = a + a
+    @test b isa Sparse
+    @test full(b) == full(a) + full(a)
+    addto!(a, a)
+    @test a isa Sparse
+    @test full(a) == full(b)
+    b = a + a
+    a .+= a
+    @test full(a) == full(b)
+
+    # Issue #114 (b): lmul! ambiguous for Sparse, breaks gclip.
+    using LinearAlgebra
+    foo(w) = (s = 0.0; for i=1:length(w); s+=w[i]; end; s)
+    w = Param(randn(2,2))
+    J = @diff foo(w)
+    @test lmul!(0.5, grad(J,w)) isa Sparse
+
+end