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 8 more kernels. #4265

Merged
merged 3 commits into from
Nov 19, 2020
Merged

Modularize 8 more kernels. #4265

merged 3 commits into from
Nov 19, 2020

Conversation

lina128
Copy link
Collaborator

@lina128 lina128 commented Nov 19, 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
Contributor

@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.

Overall looks good, one change requested to revert back to existing behaviour.

Reviewed 15 of 15 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @annxingyuan, @jinjingforever, @lina128, @mattsoulanille, and @tafsiri)


tfjs-backend-cpu/src/kernels/Multinomial.ts, line 36 at r1 (raw file):

  assertNotComplex(logits, 'multinomial');

  const $seed = seed || Math.random();

see comment below about seed at the op level. Might be worth leaving this line out until we have that sorted and tests in place.


tfjs-core/src/ops/multinomial.ts, line 69 at r1 (raw file):

  const forward: ForwardFunc<Tensor> = (backend) => {
    seed = seed || Math.random();

Let's not move this to the forward func in this PR (though I do understand why you made this change)

A few reasons

  • It turns out we don't have test for when no seed is passed. I looked at the node implementation and a NaN would be produced in undefined is passed through (webgl backend would also break, though tests wouldn't catch it). We should add some tests that don't necessarily care what the value is but that there is no error.
  • I think the way we set this (since it was originally written) actually has a bug. A user cannot manually set the seed to 0.
  • The correct default value if we are trying to match tf) for this may be 0 (at least according to my quick skim of ops.pbtxt), this may need further investigation as that may be a placeholder.

Fixing 3 and 4 might technically be breaking/change behavior in certain edge cases (even though it would be a bug fix). Probably worth separating this into a diff pr that we can fully investigate.

Copy link
Member

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


tfjs-core/src/ops/multinomial.ts, line 69 at r1 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

Let's not move this to the forward func in this PR (though I do understand why you made this change)

A few reasons

  • It turns out we don't have test for when no seed is passed. I looked at the node implementation and a NaN would be produced in undefined is passed through (webgl backend would also break, though tests wouldn't catch it). We should add some tests that don't necessarily care what the value is but that there is no error.
  • I think the way we set this (since it was originally written) actually has a bug. A user cannot manually set the seed to 0.
  • The correct default value if we are trying to match tf) for this may be 0 (at least according to my quick skim of ops.pbtxt), this may need further investigation as that may be a placeholder.

Fixing 3 and 4 might technically be breaking/change behavior in certain edge cases (even though it would be a bug fix). Probably worth separating this into a diff pr that we can fully investigate.

FYI for point 2 you can use nullish coalescing: seed = seed ?? Math.random();

Copy link
Member

@mattsoulanille mattsoulanille 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: 0 of 1 approvals obtained (waiting on @annxingyuan, @jinjingforever, @lina128, and @tafsiri)


tfjs-core/src/ops/multinomial.ts, line 69 at r1 (raw file):

Previously, mattsoulanille (Matthew Elliott Soulanille) wrote…

FYI for point 2 you can use nullish coalescing: seed = seed ?? Math.random();

Didn't mean to approve on the last comment. Oops!

Copy link
Collaborator Author

@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: 0 of 1 approvals obtained (waiting on @annxingyuan, @jinjingforever, and @tafsiri)


tfjs-backend-cpu/src/kernels/Multinomial.ts, line 36 at r1 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

see comment below about seed at the op level. Might be worth leaving this line out until we have that sorted and tests in place.

Done.


tfjs-core/src/ops/multinomial.ts, line 69 at r1 (raw file):

Previously, mattsoulanille (Matthew Elliott Soulanille) wrote…

Didn't mean to approve on the last comment. Oops!

@tafsiri Got it, reverted to previous code, added TODO to investigate as you stated. @mattsoulanille nullish coalescing and optional chaining are ES2020 specification, I think we need to change tsconfig target to es2020 to allow them to compile properly, which is beyond the scope of this PR.

Copy link
Contributor

@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.

LGTM. Thanks

Reviewed 3 of 4 files at r2.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @jinjingforever, @mattsoulanille, and @tafsiri)

Copy link
Member

@mattsoulanille mattsoulanille left a comment

Choose a reason for hiding this comment

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

Reviewed 11 of 15 files at r1, 4 of 4 files at r2.
Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @annxingyuan, @jinjingforever, @mattsoulanille, and @tafsiri)

@lina128 lina128 merged commit bca336c into tensorflow:master Nov 19, 2020
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