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

Add adjoint for pairs(::NamedTuple) and Pair #452

Merged
merged 8 commits into from Jun 16, 2020
Merged

Conversation

YingboMa
Copy link
Contributor

@YingboMa YingboMa commented Jan 9, 2020

@YingboMa
Copy link
Contributor Author

YingboMa commented Jan 9, 2020

julia> @nograd pairs

julia> Zygote.gradient(()->sum(neural_ode(dudt,x,tspan,Tsit5(),save_everystep=false,save_start=false,sensealg=BacksolveAdjoint())),p)
Grads(...)

@YingboMa YingboMa changed the title @nograd pairs Add adjoint for pairs(::NamedTuple) Jan 10, 2020
src/lib/base.jl Outdated Show resolved Hide resolved
@MikeInnes
Copy link
Member

MikeInnes commented Jan 10, 2020

Thanks! FWIW, a minimal reproducer is

julia> foo(;kw...) = 1
foo (generic function with 1 method)

julia> gradient(() -> foo(a=1,b=2.0))
ERROR: Can't differentiate gc_preserve_end expression

(This would be good to have as a test case.)

This works if all kwargs are of the same type; it would be great to understand why that is, to make sure we've got the whole issue fixed.

@YingboMa YingboMa changed the title Add adjoint for pairs(::NamedTuple) Add adjoint for pairs(::NamedTuple) and Pair Jan 10, 2020
@YingboMa
Copy link
Contributor Author

CI failure looks unrelated. @MikeInnes could you review again?

@MikeInnes
Copy link
Member

The patch looks good to me. I'd still like to understand what when wrong in detail before merging, but I can look into that myself if it's not already clear.

bors try

bors bot added a commit that referenced this pull request Jan 14, 2020
@bors
Copy link
Contributor

bors bot commented Jan 14, 2020

try

Build succeeded

@CarloLucibello
Copy link
Member

@MikeInnes what should we do here?

@cossio
Copy link
Contributor

cossio commented Jun 7, 2020

bump

@CarloLucibello
Copy link
Member

related to #669 , but of the tests in this PR

    @test (x->10*pairs((a=x, b=2))[1])'(100) === 10
    @test (x->10*pairs((a=x, b=2))[2])'(100) === 0
    foo(;kw...) = 1
    @test gradient(() -> foo(a=1,b=2.0)) === ()
    @test (x->10*(x => 2)[1])'(100) === 10
    @test (x->10*(x => 2)[2])'(100) === 0

only the 3rd passes on current master, so we should merge this as well

@CarloLucibello
Copy link
Member

@YingboMa could your rebase on master and remove the pairs adjoint from #669 ?

@CarloLucibello
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 16, 2020

Build succeeded:

@bors bors bot merged commit 389fb45 into FluxML:master Jun 16, 2020
@@ -115,3 +115,11 @@ end
end
return pairs(t), pairs_namedtuple
end

@adjoint function Base.getfield(p::Pair, i::Int)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How to write the adjoint method for LinearAlgebra.diagm of Pair{Int64,Array{Float64,1}}

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 this pull request may close these issues.

Long integrations fail with Zygote even with hardcoded adjoint
5 participants