Skip to content

Conversation

@tafsiri
Copy link
Contributor

@tafsiri tafsiri commented Jul 2, 2020

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


This change is Reviewable

Copy link
Collaborator

@lina128 lina128 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, @lina128, and @tafsiri)


tfjs-core/src/gradients/GatherV2_grad.ts, line 29 at r1 (raw file):

export const gatherGradConfig: GradConfig = {
  kernelName: GatherV2,
  inputsToSave: ['x', 'segmentIds'],

Should this be ['x', 'indices']?


tfjs-core/src/public/chained_ops/gather.ts, line 29 at r1 (raw file):

}

Tensor.prototype.gather = function<T extends Tensor>(

Curious, should we add doccomment for each chained ops? Or do we only add doccomment if the api is different than the ops, e.g. stack.

Copy link
Contributor Author

@tafsiri tafsiri 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 and @lina128)


tfjs-core/src/gradients/GatherV2_grad.ts, line 29 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Should this be ['x', 'indices']?

Fixed.


tfjs-core/src/public/chained_ops/gather.ts, line 29 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Curious, should we add doccomment for each chained ops? Or do we only add doccomment if the api is different than the ops, e.g. stack.

Chained ops are not shown in our docs so we generally don't add jsdoc to them. the jsdoc of the main op should describe how to use it chained.

You raise an interesting point about stack. My initial reaction is that we should remove stack from chaining api if it is an exception. One should be able to trivially transform non chained to chained ops without looking up docs imo.

@tafsiri tafsiri merged commit 8e86de4 into master Jul 7, 2020
@tafsiri tafsiri deleted the mod-scatter-segment branch July 7, 2020 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants