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

rm adjoint & transpose adjoints #1259

Merged
merged 2 commits into from
Jul 13, 2022
Merged

rm adjoint & transpose adjoints #1259

merged 2 commits into from
Jul 13, 2022

Conversation

mcabbott
Copy link
Member

@mcabbott mcabbott commented Jul 3, 2022

closes #1257

src/lib/array.jl Outdated
Comment on lines 365 to 366
@adjoint parent(x::LinearAlgebra.Adjoint) = parent(x), ȳ -> (LinearAlgebra.Adjoint(ȳ),)
@adjoint parent(x::LinearAlgebra.Transpose) = parent(x), ȳ -> (LinearAlgebra.Transpose(ȳ),)
Copy link
Member Author

Choose a reason for hiding this comment

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

I forgot that JuliaDiff/ChainRulesCore.jl#446 isn't merged, so removing these lines probably isn't a good idea.

Before:

julia> gradient(x -> first(parent(x)), [1,2,3]')[1]
1×3 adjoint(::Vector{Float64}) with eltype Float64:
 1.0  0.0  0.0

This PR, with tagged CRC:

julia> gradient(x -> first(parent(x)), [1,2,3]')[1]
(parent = [1.0, 0.0, 0.0],)

Copy link
Member

Choose a reason for hiding this comment

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

One alternative idea is to desguar the @adjoint and overload _pullback directly. That'll have the benefit of removing some unnecessary layers, while making it easier to do stuff like FluxML/ZygoteRules.jl#22.

Copy link
Member Author

Choose a reason for hiding this comment

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

Glad to see there were tests for this, at least: fails on isapprox(::NamedTuple{(:parent,), ...}, ::Matrix{Float64}).

@mcabbott mcabbott added the ChainRules adjoint -> rrule, and further integration label Jul 4, 2022
@mcabbott
Copy link
Member Author

Ok to merge?

Copy link
Member

@ToucheSir ToucheSir left a comment

Choose a reason for hiding this comment

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

Yes, I hadn't noticed 26ae24b

@mcabbott mcabbott merged commit ed84d53 into master Jul 13, 2022
@mcabbott mcabbott deleted the mcabbott-patch-1 branch July 13, 2022 03:23
marius311 added a commit to marius311/CMBLensing.jl that referenced this pull request Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ChainRules adjoint -> rrule, and further integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove @adjoint for adjoint/Adjoint in favor of ChainRules
2 participants