-
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
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.
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.
Just for future reference, here's the output of dumpTree
for the expression:
Command
Ident "echo"
Infix
Ident "/."
Infix
Ident "*."
Ident "gradient"
Ident "a"
Infix
Ident "^."
Ident "b"
IntLit 2
Command
Ident "echo"
Infix
Ident "/"
Infix
Ident "*"
Ident "gradient"
Ident "a"
Infix
Ident "^"
Ident "b"
IntLit 2
meaning my fears were unfounded here. Unfortunately I don't remember which case it was that behaved unexpectedly.
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.
Since it's approved should this be merged ? |
I leave it up to @Vindaar since he reviewed it. But if both of you are satisfied then it should be merged |
Thanks and sorry again for ignoring it for such a long time :) |
Hello!
This PR adds broadcasted division operation (
/.
) for the Variable type. I am quite new to the codebase so any suggestions are welcome.Thanks!