-
-
Notifications
You must be signed in to change notification settings - Fork 212
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 rule for Dict iteration #1285
Conversation
033f137
to
83cdacc
Compare
NNlib failure real but unrelated? And nightly as well. |
Nightly failure has been around for ages and should be fixed by #1280. NNlib failure I'm unsure of, need to investigate (Edit: not related, repros on NNlib master with tagged Zygote). |
CI looks green now that nightly and downstream failures have been addressed. |
What's blocking this PR? |
Any update here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks right to me.
Except we should either mutate the gradient and return nothing
Or out of place compute the change in gradient and return it to be accum
ed later.
Not both.
But this is a problem with all the Dict
code. Which is why accum(::Dict, ::Dict)
is defined as a no-op
So approving
I actually tested this after #1288 and found it broke some tests, but I don't recall which ones. Shall we continue the discussion there? |
Thanks for the PR and the review everyone! Can we get a release? |
Fixes the first example in #1065. N.B. that
_pullback
is used over@adjoint
because we're trying to get rid of it, and overrrule
because support for Dict tangents in CR is still spotty (+ we don't have to be nearly as general).