-
Notifications
You must be signed in to change notification settings - Fork 943
Cumsum #1032
Conversation
Thanks. Left a few comments - mostly how to do some transposing and rehsaping before you call the webgl shader. This will simplify the work a lot and allow summation across arbitrary axis. Reviewed 5 of 8 files at r1. src/tensor.ts, line 420 at r1 (raw file):
this jsdoc doesn't seem right. Is this taken from expandDim? Did you mean src/tensor.ts, line 423 at r1 (raw file):
the axis along which to sum. src/kernels/webgl/cumsum_gpu.ts, line 49 at r1 (raw file):
remove whitespace src/kernels/webgl/cumsum_gpu.ts, line 68 at r1 (raw file):
you can simplify this shader a bit if you do the following:
src/ops/array_ops.ts, line 1147 at r1 (raw file):
add one more code snippet with 2d and sum along axis 0 src/ops/array_ops.ts, line 1151 at r1 (raw file):
Expand a bit on what exclusive means. src/ops/array_ops.ts, line 1164 at r1 (raw file):
this won't work for axis that are not inner most unless you transpose to make the axis be inner-most, do the computation and then transpose back again. See https://github.com/tensorflow/tfjs-core/blob/master/src/ops/reduction_ops.ts#L118 for how to transpose. src/ops/array_ops_test.ts, line 2295 at r1 (raw file):
add test for 2d with axis=0 Comments from Reviewable |
added the transpose so that it works on any axis. Review status: 5 of 8 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed. src/tensor.ts, line 420 at r1 (raw file): Previously, dsmilkov (Daniel Smilkov) wrote…
Done. src/tensor.ts, line 423 at r1 (raw file): Previously, dsmilkov (Daniel Smilkov) wrote…
Done. src/kernels/webgl/cumsum_gpu.ts, line 49 at r1 (raw file): Previously, dsmilkov (Daniel Smilkov) wrote…
Done. src/kernels/webgl/cumsum_gpu.ts, line 68 at r1 (raw file): Previously, dsmilkov (Daniel Smilkov) wrote…
added transpose/undo transpose in array_ops so it applies for both cpu and gpu. src/ops/array_ops.ts, line 1147 at r1 (raw file): Previously, dsmilkov (Daniel Smilkov) wrote…
Done. src/ops/array_ops.ts, line 1151 at r1 (raw file): Previously, dsmilkov (Daniel Smilkov) wrote…
done src/ops/array_ops.ts, line 1164 at r1 (raw file): Previously, dsmilkov (Daniel Smilkov) wrote…
Done. src/ops/array_ops_test.ts, line 2295 at r1 (raw file): Previously, dsmilkov (Daniel Smilkov) wrote…
Done. Comments from Reviewable |
Reviewed 1 of 4 files at r3. src/kernels/webgl/cumsum_gpu.ts, line 3 at r4 (raw file):
2018 src/ops/array_ops.ts, line 1188 at r4 (raw file):
indent 2 more src/ops/array_ops_test.ts, line 2349 at r4 (raw file):
pull these false / true to variables for readability src/ops/array_ops_test.ts, line 2394 at r4 (raw file):
spacing here (btw if you use vscode this will auto format for you) Comments from Reviewable |
Review status: 2 of 8 files reviewed at latest revision, 12 unresolved discussions, all commit checks successful. src/kernels/webgl/cumsum_gpu.ts, line 3 at r4 (raw file): Previously, nsthorat (Nikhil Thorat) wrote…
Done. src/ops/array_ops.ts, line 1188 at r4 (raw file): Previously, nsthorat (Nikhil Thorat) wrote…
Done. src/ops/array_ops_test.ts, line 2349 at r4 (raw file): Previously, nsthorat (Nikhil Thorat) wrote…
Done. src/ops/array_ops_test.ts, line 2394 at r4 (raw file): Previously, nsthorat (Nikhil Thorat) wrote…
Done. Comments from Reviewable |
Implements cumsum operation
This change is