-
Notifications
You must be signed in to change notification settings - Fork 2k
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
initial version of gradients for prod #6216
Conversation
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.
does the cumprod uses prod gradient?
btw, yours look much simpler than TF prodGrad implementation https://github.com/tensorflow/tensorflow/blob/master/tensorflow/cc/gradients/math_grad.cc#L840, can help to explain? thanks
Reviewable status: 0 of 1 approvals obtained
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.
Does the cumprod uses prod gradient?
No, I don't think so (I would do it just with mul & div I think). But I also needed gradients for prod so I needed to add it. I need einsum gradients also... so that might come next :)
yours look much simpler than TF prodGrad implementation ... can help to explain?
I thought I knew, but as I look at it more, I'm confused. I need to think about this more, I'm pretty sure I've made a mistake...
Reviewable status: 0 of 1 approvals obtained
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.
Actually, I'm sure I've made a mistake... I'll come back to you on this with an update.
Reviewable status: 0 of 1 approvals obtained
ee97547
to
c19bc56
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.
OK, I went back to this I re-implemented it using, cumprod, as originally planned (in my new baby brain fuzz state I had gotten myself confused and thought I had a massive simplification, but I didn't).
To debug, I also made a tinkerable/easier to debug/tweak version in: https://codesandbox.io/s/iislucas-tfjs-prod-grad-example-n7i7ug?file=/src/index.ts
And one to run tests more or less copy/pasted from the prod_test.ts file:
https://codesandbox.io/s/iislucas-tfjs-prods-grad-tests-gpl0oz?file=/src/index.ts
I think tests pass now, and I have the use-case from the c-implementation as a test case also.
PTAL.
Reviewable status: 0 of 1 approvals obtained
cf685ed
to
93f4cf3
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.
Cool, thank you for implementing this, looks like the c code support multiple axis as you have also addressed in your implementation.
The implementation looks very easy to following. I just have couple minor comments. Thanks!
Reviewable status: 0 of 1 approvals obtained (waiting on @iislucas)
tfjs-core/src/gradients/Prod_grad.ts, line 29 at r2 (raw file):
// Works on a single axis. function prodGradFn_(x: Tensor, dy: Tensor, axis: number): Tensor { // The gradent tensor (dy) has a set of axis removed, so we create re-shaped
couple typo: gradient
and set of axes
tfjs-core/src/ops/prod_test.ts, line 112 at r2 (raw file):
// Illustrative example from the C++ implementation for comparison. See: // https://github.com/tensorflow/tensorflow/blob/5dee95f8bafe3f342693d2135da451283b1fbaec/tensorflow/cc/gradients/math_grad.cc it('gradients: prod(multi-dimensional tensor with zeros)', async () => {
can you add a test that covers multiple reduction axes? thanks
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.
Thanks! Done.
Reviewable status: 0 of 1 approvals obtained (waiting on @pyu10055)
tfjs-core/src/gradients/Prod_grad.ts, line 29 at r2 (raw file):
Previously, pyu10055 (Ping Yu) wrote…
couple typo:
gradient
andset of axes
oops, yes, I had axis instead of axes in several places. fixed.
DONE.
tfjs-core/src/ops/prod_test.ts, line 112 at r2 (raw file):
Previously, pyu10055 (Ping Yu) wrote…
can you add a test that covers multiple reduction axes? thanks
There was one that worked on all axes already, but I added one also for working on two axes at once also.
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.
Reviewable status: 0 of 1 approvals obtained (waiting on @pyu10055)
tfjs-core/src/ops/prod_test.ts, line 112 at r2 (raw file):
Previously, iislucas (iislucas) wrote…
There was one that worked on all axes already, but I added one also for working on two axes at once also.
Done.
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.
Reviewable status:
complete! 1 of 1 approvals obtained
Initial implementation of gradients for
prod
operator:Fixes: #2587
Tested locally with:
This change is