-
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 broadcastTo op #2919
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, @dsmilkov, @lina128, and @tafsiri)
tfjs-core/src/kernel_names.ts, line 43 at r1 (raw file):
export type BroadcastToInputs = Pick<NamedTensorInfoMap, 'x'>; export interface BroadCastToAttrs { reps: number[];
Reps feels like more of an implementation detail - maybe the attrs could still be shape, and then within the kernels themselves we could derive reps?
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.
+1 to Ann's comment. Nothing else. Nice!!
Reviewed 14 of 14 files at r1.
Reviewable status: complete! 2 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, and @tafsiri)
tfjs-core/src/kernel_names.ts, line 43 at r1 (raw file):
Previously, annxingyuan (Ann Yuan) wrote…
Reps feels like more of an implementation detail - maybe the attrs could still be shape, and then within the kernels themselves we could derive reps?
+1. this should be shape
(whatever attrs are in the public API of broadcastTo)
tfjs-core/src/ops/broadcast_to.ts, line 3 at r1 (raw file):
/** * @license * Copyright 2018 Google Inc. All Rights Reserved.
2020 here and elsewhere where you have 2018 and 2019
tfjs-core/src/public/chained_ops/register_all_chained_ops_test.ts, line 23 at r1 (raw file):
// Testing for presence of chained op in this file will allow us to more easily // customize when we want this test to run. Currently it will run be default // (And kerma will always load the chain augmentor files). But this gives us
Not for this PR, just thoughts: karma loads all the chained ops, but node (yarn test-node
) won't. So ideally we would have one test file for each chained op, where we import that specific chained op at the top of the test file and test for its existence. basically testing public API for importing chained ops. And if something is off, the node test will fail.
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.
Since the remaining comment is small and has clear outcome, LGTM since I don't need to take another look.
Modularizes the broadCastTo op without updating kernels. (as previously discussed with Ann and Na)
A few things emerged while doing this.
tile
op but the backward mode is a different computation than the one of tile. This requires declaring a new kernel for broadcastTo that backends will have to implement (that they use tile is an implementation detail internal to them). Right now that kernel does not exist.To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.
This change is