-
Notifications
You must be signed in to change notification settings - Fork 2k
[webgl] modularize kernels #4268
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
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, @mattsoulanille, and @tafsiri)
tfjs-backend-webgl/src/kernels/BatchToSpaceND.ts, line 79 at r1 (raw file):
kernelName: BatchToSpaceND, backendName: 'webgl', kernelFunc: batchToSpaceND as {} as KernelFunc
These unsafe casts to KernelFunc might not be necessary in all cases if we can declare the function as a KernelFunc in its definition: export const batchToSpaceND: KernelFunc = (...
. I haven't tried it with this kernel, but it works for the Real.ts
kernel, so I'm guessing it probably generalizes.
LGTM modulo the above nit.
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-webgl/src/kernels/BatchToSpaceND.ts, line 79 at r1 (raw file):
Previously, mattsoulanille (Matthew Elliott Soulanille) wrote…
These unsafe casts to KernelFunc might not be necessary in all cases if we can declare the function as a KernelFunc in its definition:
export const batchToSpaceND: KernelFunc = (...
. I haven't tried it with this kernel, but it works for theReal.ts
kernel, so I'm guessing it probably generalizes.LGTM modulo the above nit.
I think we've found in the past that it produces a different set of casts in the kernel func definition itself so went with this convention. I'm not sure what you mean by it works in the Real.ts kernel, i'm looking at that file and not seeing what you are describing.
However one thing your comment did make me notice is that I shouldn't use a const variable for this function. We should prefer named functions where possible and in this case it is quite possible so I'll make that change.
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-webgl/src/kernels/BatchToSpaceND.ts, line 79 at r1 (raw file):
Previously, tafsiri (Yannick Assogba) wrote…
I think we've found in the past that it produces a different set of casts in the kernel func definition itself so went with this convention. I'm not sure what you mean by it works in the Real.ts kernel, i'm looking at that file and not seeing what you are describing.
However one thing your comment did make me notice is that I shouldn't use a const variable for this function. We should prefer named functions where possible and in this case it is quite possible so I'll make that change.
By "it works in the Real.ts
kernel," I meant that, as a local test, I refactored that kernel to conform with my comment. Now that I reread my comment, I realize it reads as if I'm saying Real.ts
already conforms to my recommended changes. My bad!
Good point about casting directly to KernelFunc. I forgot we may need to use those functions outside of the KernelConfig, and if they're already cast, we don't know their type.
We can probably remove the casts in the KernelConfig definition, though, since batchToSpaceND (and other kernel implementations) should be a subtype of KernelFunc (I think).
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.
Thank you Yannick. LGTM
Reviewable status:
complete! 2 of 1 approvals obtained (waiting on @annxingyuan and @lina128)
Modularize pad, batchToSpaceND, spacetoBatchND, LRN, LRNGrad.
To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.
This change is