- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 216
 
Add rule for Dict iteration #1285
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
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 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
          
 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
_pullbackis used over@adjointbecause we're trying to get rid of it, and overrrulebecause support for Dict tangents in CR is still spotty (+ we don't have to be nearly as general).