-
Notifications
You must be signed in to change notification settings - Fork 2k
modularize unary ops #3586
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 unary ops #3586
Conversation
annxingyuan
left a 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.
Reviewable status:
complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @lina128, and @tafsiri)
tfjs-backend-wasm/src/kernels/ClipByValue.ts, line 25 at r1 (raw file):
function setup(backend: BackendWasm) { wasmClip = backend.wasm.cwrap('ClipByValue', null /* void */, [
Use symbol here
tfjs-core/src/gradients/ClipByValue_grad.ts, line 33 at r1 (raw file):
const {clipValueMin, clipValueMax} = attrs as {} as ClipByValueAttrs; return { // tslint:disable-next-line: no-unnecessary-type-assertion
Is this tslint needed?
tfjs-core/src/ops/erf.ts, line 46 at r1 (raw file):
() => 'Input dtype must be `int32` or `float32`.'); if ($x.dtype === 'int32') {
sorry if I'm misremembering - did we decide that casts should go into the forwardfunc?
tafsiri
left a 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.
Reviewable status:
complete! 1 of 1 approvals obtained (waiting on @annxingyuan and @lina128)
tfjs-backend-wasm/src/kernels/ClipByValue.ts, line 25 at r1 (raw file):
Previously, annxingyuan (Ann Yuan) wrote…
Use symbol here
Done. I don't know if we've doing this so far and it isn't strictly required but it think its a good idea to reduce variance in naming. Should we do this in other kernels as well?
tfjs-core/src/gradients/ClipByValue_grad.ts, line 33 at r1 (raw file):
Previously, annxingyuan (Ann Yuan) wrote…
Is this tslint needed?
Nope. And i removed the chained where. Done
tfjs-core/src/ops/erf.ts, line 46 at r1 (raw file):
Previously, annxingyuan (Ann Yuan) wrote…
sorry if I'm misremembering - did we decide that casts should go into the forwardfunc?
Generally yes, but in this case, according to ops.pbtxt, erf does not take int tensors. So this cast is a convenience of the tfjs public api
annxingyuan
left a 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.
Reviewable status:
complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @lina128, and @tafsiri)
tfjs-backend-wasm/src/kernels/ClipByValue.ts, line 25 at r1 (raw file):
Previously, tafsiri (Yannick Assogba) wrote…
Done. I don't know if we've doing this so far and it isn't strictly required but it think its a good idea to reduce variance in naming. Should we do this in other kernels as well?
Yeah I think it would be a good idea. This can be one of the things we do in the final WASM cleanup PR, along with making sure WASM kernels are using interfaces from kernel_names.ts rather than defining their own.
modularize, clipByValue, erf, exp, expm1, log, log1p, logSigmoid, reciprocal
To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.
This change is