Skip to content

Conversation

@tafsiri
Copy link
Contributor

@tafsiri tafsiri commented Jun 10, 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 review from annxingyuan and lina128 June 10, 2020 22:37
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 and @tafsiri)


tfjs-backend-wasm/src/kernels/ResizeBilinear.ts, line 44 at r1 (raw file):

function resizeBilinear(
    params: {backend: {}, inputs: NamedTensorInfoMap, attrs: NamedAttrMap}):

Hi Yannick, other kernels use args, do we have to change all the other ones to params? Also why can't backend type be BackendWasm, same for inputs and attrs.

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-backend-wasm/src/kernels/ResizeBilinear.ts, line 44 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Hi Yannick, other kernels use args, do we have to change all the other ones to params? Also why can't backend type be BackendWasm, same for inputs and attrs.

Hmm, i was getting a compiler error at some point but no longer with some of these.

I've changed params back to args and the type of backend to BackendWasm,

However if inputs and attrs are the ResizeBilinear* types then registerKernel won't compile because the types don't overlap with the signature of kernelFunc specifies, same reasons we cast attrs to {} before casting back to ResizeBilinearAttrs. Tbh i'm not 100% sure why this worked before, my suspicion is that because ResizeBilinearInputs and ResizeBilinearAttrs were defined within this file as opposed to being imported from core.

Good catch.

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


tfjs-backend-wasm/src/kernels/ResizeBilinear.ts, line 44 at r1 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

Hmm, i was getting a compiler error at some point but no longer with some of these.

I've changed params back to args and the type of backend to BackendWasm,

However if inputs and attrs are the ResizeBilinear* types then registerKernel won't compile because the types don't overlap with the signature of kernelFunc specifies, same reasons we cast attrs to {} before casting back to ResizeBilinearAttrs. Tbh i'm not 100% sure why this worked before, my suspicion is that because ResizeBilinearInputs and ResizeBilinearAttrs were defined within this file as opposed to being imported from core.

Good catch.

Hmm, that's interesting. This kernel that I recently touched compiles ok with input and attr type from kernel name: https://github.com/tensorflow/tfjs/blob/master/tfjs-backend-wasm/src/kernels/AvgPool.ts#L48

@tafsiri tafsiri merged commit cff2f68 into master Jun 12, 2020
@tafsiri tafsiri deleted the mod-resize-ops branch June 12, 2020 17:00
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