Skip to content

Conversation

@tafsiri
Copy link
Contributor

@tafsiri tafsiri commented Jun 17, 2020

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


This change is Reviewable

@tafsiri tafsiri requested a review from lina128 June 17, 2020 03:06
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.

Reviewed 29 of 35 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @lina128 and @tafsiri)


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

    const [$x] = saved;
    return {
      // tslint:disable-next-line: no-unnecessary-type-assertion

Does ts starts to throw error now?


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

  // TODO(julianoks): Return null for condition gradient
  // when backprop supports it.
  const grad = (dy: T, saved: Tensor[]) => {

Should the grad be modularized?

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: 0 of 1 approvals obtained (waiting on @lina128)


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

Previously, lina128 (Na Li) wrote…

Does ts starts to throw error now?

in this case tslint believes the type assertion is unnecessary, but if you remove it tsc will not compile the file.


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

Previously, lina128 (Na Li) wrote…

Should the grad be modularized?

Good catch, forgot to delete it here. Done.

@tafsiri tafsiri requested a review from lina128 June 17, 2020 17:32
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 @lina128)


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

Previously, tafsiri (Yannick Assogba) wrote…

in this case tslint believes the type assertion is unnecessary, but if you remove it tsc will not compile the file.

That's weird, because the code was there before, meaning it compiles before~

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


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

Previously, lina128 (Na Li) wrote…

That's weird, because the code was there before, meaning it compiles before~

True, but this may just be some weakness in the linter (note that its not the compiler complaining)

@tafsiri tafsiri merged commit e579e61 into master Jun 17, 2020
@tafsiri tafsiri deleted the mod-logical-ops branch June 17, 2020 20:24
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