-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Modularize cumsum. #3296
Modularize cumsum. #3296
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.
Looks great!
Reviewed 14 of 14 files at r1.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @lina128, and @tafsiri)
tfjs-core/src/ops/cumsum.ts, line 59 at r1 (raw file):
const forward: ForwardFunc<Tensor> = (backend: KernelBackend, save: GradSaveFunc) => { axis = axis | 0;
nit: is this needed given the default argument?
tfjs-core/src/ops/image_ops_test.ts, line 178 at r1 (raw file):
it('works with other operators, div', async () => { // This test ensures that asynchronous backends work with NMS, which
nit (optional) thanks for adding this. I was thinking about and wondering if there is just a better name for this test. If you can think of one feel free to add it. It seems really be about where the data for the inputs resides than other operators per-se
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 (waiting on @lina128)
tfjs-core/src/ops/cumsum.ts, line 59 at r1 (raw file):
Previously, tafsiri (Yannick Assogba) wrote…
nit: is this needed given the default argument?
Done
tfjs-core/src/ops/image_ops_test.ts, line 178 at r1 (raw file):
Previously, tafsiri (Yannick Assogba) wrote…
nit (optional) thanks for adding this. I was thinking about and wondering if there is just a better name for this test. If you can think of one feel free to add it. It seems really be about where the data for the inputs resides than other operators per-se
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! 2 of 1 approvals obtained (waiting on @annxingyuan and @lina128)
tfjs-core/src/gradients/Cumsum_grad.ts, line 29 at r2 (raw file):
const [x] = saved; const cumsumAttrs: CumsumAttrs = attrs as {} as CumsumAttrs; const {axis, exclusive, reverse} = cumsumAttrs;
nit: Can be one line: {axis, exclusive, reverse} = attrs as {} as CumsumAttrs;
tfjs-core/src/gradients/Cumsum_grad.ts, line 35 at r2 (raw file):
const permutation = getAxesPermutation([axis], x.rank); let out = dy.cumsum(axis, exclusive, !reverse);
remove chained op.
tfjs-core/src/gradients/Cumsum_grad.ts, line 38 at r2 (raw file):
if (permutation != null) { out = out.transpose(permutation);
remove chained op.
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! 2 of 1 approvals obtained (waiting on @lina128)
tfjs-core/src/gradients/Cumsum_grad.ts, line 29 at r2 (raw file):
Previously, lina128 (Na Li) wrote…
nit: Can be one line: {axis, exclusive, reverse} = attrs as {} as CumsumAttrs;
Done
tfjs-core/src/gradients/Cumsum_grad.ts, line 35 at r2 (raw file):
Previously, lina128 (Na Li) wrote…
remove chained op.
Done
tfjs-core/src/gradients/Cumsum_grad.ts, line 38 at r2 (raw file):
Previously, lina128 (Na Li) wrote…
remove chained op.
Done
To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.
This change is