-
Notifications
You must be signed in to change notification settings - Fork 95
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 division gate #583
base: master
Are you sure you want to change the base?
Add division gate #583
Conversation
Sorry this didn't receive any attention since January! I've unfortunately also been too busy and kept away from Github. Thanks for the contribution! It generally looks good to me, aside from that minor comment. |
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.
Looks good to me, just want to make sure you are certain the behavior is as expected. :) Marking it as approved anyway already!
let gradient = payload.variable.grad | ||
result = newDiffs[TT](2) | ||
result[0] = gradient /. self.b.value | ||
result[1] = - gradient *. self.a.value /. self.b.value ^. 2 |
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.
I remember there being many weird edge cases with the precedence of ^
(and thus likely ^.
) in the Nim parser. Did you check that it binds correctly to self.b.value
only and doesn't for some bizarre reason bind later to the result of the division? I've had hard to understand bugs due to that in contexts of using unchained
iirc.
let gradC = ones[float32](2, 4) | ||
|
||
check: va.grad == gradC /. b | ||
check: vb.grad == - gradC *. a /. b ^. 2 |
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.
Related to my comment above: if the binding were broken up there it would be here too, making the test work despite doing the wrong thing.
Hello!
This PR adds broadcasted division operation (
/.
) for the Variable type. I am quite new to the codebase so any suggestions are welcome.Thanks!