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

deprecate Zygote.@nograd #1249

Merged
merged 2 commits into from
Jul 30, 2022
Merged

deprecate Zygote.@nograd #1249

merged 2 commits into from
Jul 30, 2022

Conversation

mzgubic
Copy link
Collaborator

@mzgubic mzgubic commented Jun 23, 2022

Closes #1235

Needs JuliaDiff/ChainRules.jl#637 to work.

The controversial thing is changing the tests in test/lib/number.jl. This is what ChainRules does, though I am not 100% certain why. In any case, if we decide against following the ChainRules convention here, it's possible to do it.

@ToucheSir
Copy link
Member

Hard to tell since CI won't run until CR 1.37 is tagged, but using @non_differentiable over @nograd can cause some inference hiccups. See e.g. https://github.com/FluxML/Zygote.jl/pull/1248/files#diff-1040a6fce812f91964b258de2a02fb69296c75a9701c3326df2671ab9ea7e5f0R35. I don't think that'll be a problem for the math functions here, but internals like Buffer might be trickier.

@mcabbott mcabbott added the ChainRules adjoint -> rrule, and further integration label Jul 4, 2022
@mzgubic mzgubic closed this Jul 7, 2022
@mzgubic mzgubic reopened this Jul 7, 2022
@mzgubic
Copy link
Collaborator Author

mzgubic commented Jul 8, 2022

Is CI passing good enough or do we want some manual inference checks here?

@mzgubic
Copy link
Collaborator Author

mzgubic commented Jul 27, 2022

bump

ToucheSir
ToucheSir approved these changes Jul 30, 2022
View reviewed changes
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.

Thanks! There was one rule where I found @nograd inferred better than @non_differentiable, but I can't think of it now and we can always write a manual _pullback for it if need be.

@ToucheSir ToucheSir merged commit cb59b6c into FluxML:master Jul 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ToucheSir ToucheSir ToucheSir approved these changes

Assignees
No one assigned
Labels
ChainRules adjoint -> rrule, and further integration
Projects
None yet
Milestone
No milestone
3 participants
@mzgubic @ToucheSir @mcabbott