Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minimizing sum fails #1510

Closed
cossio opened this issue Feb 16, 2021 · 5 comments · Fixed by #1613
Closed

Minimizing sum fails #1510

cossio opened this issue Feb 16, 2021 · 5 comments · Fixed by #1613

Comments

@cossio
Copy link
Contributor

cossio commented Feb 16, 2021

T = randn(10,10)
ps = Flux.params(T)
opt=ADAM()
for iter = 1:100
    gs = gradient(ps) do
        sum(T)
    end
    Flux.update!(opt, ps, gs)
end

Throws the following error:

ArgumentError: Cannot setindex! to 0.0009999999900000003 for an AbstractFill with value 1.0.

Stacktrace:
[1] setindex! at /home/cossio/.julia/packages/FillArrays/tE9Xq/src/FillArrays.jl:41 [inlined]
[2] _setindex! at ./abstractarray.jl:1176 [inlined]
[3] setindex! at ./abstractarray.jl:1153 [inlined]
[4] macro expansion at ./broadcast.jl:932 [inlined]
[5] macro expansion at ./simdloop.jl:77 [inlined]
[6] copyto! at ./broadcast.jl:931 [inlined]
[7] copyto! at ./broadcast.jl:886 [inlined]
[8] materialize! at ./broadcast.jl:848 [inlined]
[9] materialize!(::FillArrays.Fill{Float64,2,Tuple{Base.OneTo{Int64},Base.OneTo{Int64}}}, ::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{2},Nothing,typeof(*),Tuple{Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{2},Nothing,typeof(/),Tuple{Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{2},Nothing,typeof(/),Tuple{Array{Float64,2},Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{0},Nothing,typeof(-),Tuple{Int64,Float64}}}},Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{2},Nothing,typeof(+),Tuple{Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{2},Nothing,typeof(sqrt),Tuple{Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{2},Nothing,typeof(/),Tuple{Array{Float64,2},Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{0},Nothing,typeof(-),Tuple{Int64,Float64}}}}}},Float64}}}},Float64}}) at ./broadcast.jl:845
[10] apply!(::ADAM, ::Array{Float64,2}, ::FillArrays.Fill{Float64,2,Tuple{Base.OneTo{Int64},Base.OneTo{Int64}}}) at /home/cossio/.julia/packages/Flux/05b38/src/optimise/optimisers.jl:177
[11] update!(::ADAM, ::Array{Float64,2}, ::FillArrays.Fill{Float64,2,Tuple{Base.OneTo{Int64},Base.OneTo{Int64}}}) at /home/cossio/.julia/packages/Flux/05b38/src/optimise/train.jl:23
[12] update!(::ADAM, ::Params, ::Zygote.Grads) at /home/cossio/.julia/packages/Flux/05b38/src/optimise/train.jl:29
[13] top-level scope at In[89]:8
[14] include_string(::Function, ::Module, ::String, ::String) at ./loading.jl:1091

@cossio
Copy link
Contributor Author

cossio commented Feb 16, 2021

@CarloLucibello
Copy link
Member

I think we just have to handle Fills properly in apply!

@DhairyaLGandhi
Copy link
Member

This is likely a regression in NNlib or Zygote, because we haven't broken the optimisation in here

@cossio
Copy link
Contributor Author

cossio commented Feb 16, 2021

I don't think Zygote is the issue, since the gradient is properly computed. Maybe apply! should dispatch on the type of gs, and mutate only if appropriate.

For instance, something like

apply!(opt, x, g::AbstractArray) = apply!(opt, x, Array(g)) # copy g if it is not mutable
apply!(opt, x, g::Array) = ... # normal code here that mutates g

@DhairyaLGandhi
Copy link
Member

DhairyaLGandhi commented Mar 11, 2021

I think its that Fill doesn't want to eagerly collect, which is great, but it can lead to edge cases

bors bot added a commit that referenced this issue Jun 17, 2021
1613: use ArrayInterface.restructure in update! r=CarloLucibello a=CarloLucibello

Suggestion coming from @ChrisRackauckas in FluxML/Zygote.jl#989. 
Now `update!` handles basically any gradient Zygote emits, e.g. FillArrays and Zygote.OneElement. 

Fix #1510 


Co-authored-by: CarloLucibello <[email protected]>
@bors bors bot closed this as completed in de76e08 Jun 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants