-
Notifications
You must be signed in to change notification settings - Fork 5
Add tests/fixes for inplace rules #158
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
|
I'd like to do a bit of tidying up with this one but it can be merged if tests pass and people feel like it, I can just make a separate PR. |
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
8a1283e to
772b61b
Compare
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.
Overall looks good to me!
I have two remaining questions:
- This also bumps the Mooncake version, should this come before or after #164 ?
I might be getting this wrong, but I am kind of confused now about why we arezero!ing thedargs. Aren't we supposed to accumulate the contributions of the gradient into that, not setting them to? I.e., should this not just bedarg1 += zero(darg1), or effectively we don't have to do that?
[EDIT] ignore this last part, I'm stupid.
facdda6 to
4528a73
Compare
Added more tests to
test_pullbacks_matchto make sure the state of the arguments is restored, and the final argument derivatives match between inplace and non in place methods.Unfortunately, the Mooncake FD tester doesn't work well for our functions, because
Abecomes a scratch space, and the inputs are also the outputs (so get incremented twice under the FD scheme).