-
Notifications
You must be signed in to change notification settings - Fork 2k
[core] In gradient mode, if fused activation is not backproppable, call unfused ops instead. #2258
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
Conversation
nsthorat
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.
Reviewed 3 of 3 files at r1, 1 of 1 files at r2.
Reviewable status:complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, and @nsthorat)
tfjs-core/src/ops/fused_ops.ts, line 502 at r1 (raw file):
preluActivationWeights?: Tensor }): T { if (shouldNotFuse(ENGINE.state.gradientDepth, activation)) {
lets make this the inverse: shouldFuse
tfjs-core/src/ops/fused_test.ts, line 952 at r2 (raw file):
}); // tslint:disable-next-line:max-line-length
i think you can remove this tslint
tfjs-core/src/ops/fused_test.ts, line 953 at r2 (raw file):
// tslint:disable-next-line:max-line-length it('calling fused op in gradient mode with activation that does not support fused gradients works',
instead of "that does not support fused gradient" how about we just have tests for all the combinations and dont talk about whether internally we support things
tfjs-core/src/ops/fused_util.ts, line 45 at r1 (raw file):
// Returns gradient for fused activation. export const getDyActivation =
put "fused" in the name
also, optional: should this just live in the other file?
tfjs-core/src/ops/fused_util.ts, line 59 at r1 (raw file):
// Returns gradient for fused bias. export const getBiasGradient = (bias: Tensor, dyActivation: Tensor): Tensor => {
getFusedBiasGradient
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 @dsmilkov)
tfjs-core/src/ops/fused_ops.ts, line 502 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
lets make this the inverse: shouldFuse
Done
tfjs-core/src/ops/fused_test.ts, line 952 at r2 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
i think you can remove this tslint
Done
tfjs-core/src/ops/fused_test.ts, line 953 at r2 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
instead of "that does not support fused gradient" how about we just have tests for all the combinations and dont talk about whether internally we support things
Done
tfjs-core/src/ops/fused_util.ts, line 45 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
put "fused" in the name
also, optional: should this just live in the other file?
Done
tfjs-core/src/ops/fused_util.ts, line 59 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
getFusedBiasGradient
Done
This PR fixes #2194
Changes
To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.
This change is