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 rule for Dict iteration #1285

Merged
merged 1 commit into from
Sep 5, 2022
Merged

Add rule for Dict iteration #1285

merged 1 commit into from
Sep 5, 2022

Conversation

ToucheSir
Copy link
Member

Fixes the first example in #1065. N.B. that _pullback is used over @adjoint because we're trying to get rid of it, and over rrule because support for Dict tangents in CR is still spotty (+ we don't have to be nearly as general).

@CarloLucibello
Copy link
Member

CarloLucibello commented Aug 11, 2022

NNlib failure real but unrelated? And nightly as well.

@ToucheSir
Copy link
Member Author

ToucheSir commented Aug 12, 2022

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).

@ToucheSir
Copy link
Member Author

CI looks green now that nightly and downstream failures have been addressed.

@mohamed82008
Copy link
Contributor

What's blocking this PR?

@mohamed82008
Copy link
Contributor

Any update here?

Copy link
Member

@oxinabox oxinabox left a 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 accumed 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

@ToucheSir ToucheSir merged commit 304681a into master Sep 5, 2022
@ToucheSir ToucheSir deleted the bc/iterate-dict-rule branch September 5, 2022 21:49
@ToucheSir
Copy link
Member Author

Except we should either mutate the gradient and return nothing
Or out of place compute the change in gradient and return it to be accumed later.
Not both.

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?

@mohamed82008
Copy link
Contributor

Thanks for the PR and the review everyone! Can we get a release?

@ToucheSir
Copy link
Member Author

JuliaRegistries/General#67749

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants