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.

Sign up for GitHub

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

Jump to bottom

Fix Flux.flip by providing an adjoint for Base.reverse #515

Merged
merged 3 commits into from
Feb 25, 2020
Merged

Fix Flux.flip by providing an adjoint for Base.reverse #515

merged 3 commits into from
Feb 25, 2020

Conversation

tanhevg
Copy link
Contributor

@tanhevg tanhevg commented Feb 14, 2020

The main motivation behind this PR is to address various issues concerning Flux.flip() (used mainly for bRNNs), e.g. FluxML/Flux.jl#962, FluxML/Flux.jl#990 and FluxML/model-zoo#179

@tanhevg tanhevg requested a review from MikeInnes February 14, 2020 12:29
@CarloLucibello
Copy link
Member

Does it fix flip? Can we have a test for it?

@tanhevg
Copy link
Contributor Author

tanhevg commented Feb 14, 2020

@CarloLucibello

Does it fix flip? Can we have a test for it?

Yes it does. Tests would have to go in Flux, so this would be a separate PR. One of us could put one in once this is merged and tagged in a Zygote release. Adding PRs that rely on unreleased versions of dependencies is messy.

@tanhevg tanhevg changed the title Adjoint for Base.reverse Fix Flux.flip by providing an adjoint for Base.reverse Feb 14, 2020
@DhairyaLGandhi
Copy link
Member

Having the PR up would be helpful, I think it's one of the tail end of fixes remaining to update the zoo as well

@CarloLucibello
Copy link
Member

@CarloLucibello

Does it fix flip? Can we have a test for it?

Yes it does. Tests would have to go in Flux, so this would be a separate PR. One of us could put one in once this is merged and tagged in a Zygote release. Adding PRs that rely on unreleased versions of dependencies is messy.

oh right, thought flip was a Base function

@tanhevg tanhevg removed the request for review from MikeInnes February 14, 2020 13:57
MikeInnes
MikeInnes reviewed Feb 17, 2020
View reviewed changes
Copy link
Member

@MikeInnes MikeInnes left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

src/lib/array.jl Outdated
@adjoint function reverse(x::AbstractArray, args...; kwargs...)
_reverse(t) = reverse(t, args...; kwargs...)
_nothings(t) = map(_->nothing, keys(t))
_reverse(x), Δ->(_reverse(Δ), _nothings(args)..., _nothings(kwargs)...)
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to return adjoints for the kwargs here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, duly noted

@CarloLucibello
Copy link
Member

this seems good to go

@DhairyaLGandhi
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 25, 2020

Build succeeded

@bors bors bot merged commit 701135b into FluxML:master Feb 25, 2020
@CarloLucibello CarloLucibello mentioned this pull request Feb 25, 2020
Closed
@tanhevg
Copy link
Contributor Author

tanhevg commented Feb 26, 2020

Fix FluxML/Flux.jl#962

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MikeInnes MikeInnes MikeInnes left review comments

@CarloLucibello CarloLucibello CarloLucibello approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tanhevg @CarloLucibello @DhairyaLGandhi @MikeInnes