Skip to content

Conversation

@jinjingforever
Copy link
Collaborator

@jinjingforever jinjingforever commented Sep 14, 2020

This change is Reviewable

@jinjingforever jinjingforever marked this pull request as ready for review September 14, 2020 23:55
@jinjingforever
Copy link
Collaborator Author

After this PR, I should be able to add the rest of the ops in batches:) Thanks for all the help

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.

LGTM

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @annxingyuan and @tafsiri)

@jinjingforever jinjingforever merged commit abb4ba3 into master Sep 15, 2020
@jinjingforever jinjingforever deleted the modularize branch September 15, 2020 16:21
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 @jinjingforever and @tafsiri)


tfjs-backend-webgl/src/kernels/Cos.ts, line 26 at r2 (raw file):

  kernelName: Cos,
  backendName: 'webgl',
  kernelFunc: ({inputs, backend}) => {

Hi Jing, please also export cos's kernel function, this will allow other kernels to call the kernel func directly. If calling through cosConfig.kernelFunc, it only has generic type, i.e. NamedTensorMap, as oppose to CosInputs. Because of this and less code to type, we should always export two things for each kernel, one kernelFunc and one config.

Copy link
Collaborator Author

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


tfjs-backend-webgl/src/kernels/Cos.ts, line 26 at r2 (raw file):

Previously, lina128 (Na Li) wrote…

Hi Jing, please also export cos's kernel function, this will allow other kernels to call the kernel func directly. If calling through cosConfig.kernelFunc, it only has generic type, i.e. NamedTensorMap, as oppose to CosInputs. Because of this and less code to type, we should always export two things for each kernel, one kernelFunc and one config.

ah got it. Thanks! Will fix this in my upcoming PRs.

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.

4 participants