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 division gate #583

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add division gate #583

wants to merge 1 commit into from

Conversation

jegork
Copy link

@jegork jegork commented Jan 8, 2023

Hello!

This PR adds broadcasted division operation (/.) for the Variable type. I am quite new to the codebase so any suggestions are welcome.

Thanks!

@Vindaar
Copy link
Collaborator

Vindaar commented Apr 3, 2023

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.

Copy link
Collaborator

@Vindaar Vindaar left a 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
Copy link
Collaborator

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
Copy link
Collaborator

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.

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

Successfully merging this pull request may close these issues.

None yet

2 participants