Skip to content
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

Merged
merged 7 commits into from
Mar 18, 2020
Merged

modularize broadcastTo op #2919

merged 7 commits into from
Mar 18, 2020

Conversation

tafsiri
Copy link
Contributor

@tafsiri tafsiri commented Mar 17, 2020

Modularizes the broadCastTo op without updating kernels. (as previously discussed with Ann and Na)

A few things emerged while doing this.

  • Since the forward mode of broadcastTo currently just uses the 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.
  • The grad func of broadCastTo requires attrs, so this has been added to the gradFunc interface and to the tape mechanism for computing gradients.
  • broadcastTo was not a chained op, nor in the backend interface, so I added it as a chained op for consistency.
  • Added a place to test that chained ops exist on tensor instances.
  • There is some temporary casting from to and from the unknown type because NamedTensorMap and NamedTensorInfoMap do not overlap. Once we switch to runKernel in future both the ops and kernels will likely be using NamedTensorInfoMap and we can reduce the type mismatch.

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

@tafsiri tafsiri changed the title WIP modularize broadcastTo op modularize broadcastTo op Mar 17, 2020
@tafsiri tafsiri marked this pull request as ready for review March 17, 2020 14:53
Copy link
Contributor

@annxingyuan annxingyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: 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?

Copy link
Contributor

@dsmilkov dsmilkov left a 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: :shipit: 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.

Copy link
Contributor

@dsmilkov dsmilkov left a 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.

@tafsiri tafsiri merged commit f8e3007 into master Mar 18, 2020
@tafsiri tafsiri deleted the modularise-broadcastTo-op branch March 18, 2020 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants