Skip to content

Conversation

tafsiri
Copy link
Contributor

@tafsiri tafsiri commented Nov 19, 2020

Modularize pad, batchToSpaceND, spacetoBatchND, LRN, LRNGrad.

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


This change is Reviewable

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


tfjs-backend-webgl/src/kernels/BatchToSpaceND.ts, line 79 at r1 (raw file):

  kernelName: BatchToSpaceND,
  backendName: 'webgl',
  kernelFunc: batchToSpaceND as {} as KernelFunc

These unsafe casts to KernelFunc might not be necessary in all cases if we can declare the function as a KernelFunc in its definition: export const batchToSpaceND: KernelFunc = (.... I haven't tried it with this kernel, but it works for the Real.ts kernel, so I'm guessing it probably generalizes.

LGTM modulo the above nit.

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-webgl/src/kernels/BatchToSpaceND.ts, line 79 at r1 (raw file):

Previously, mattsoulanille (Matthew Elliott Soulanille) wrote…

These unsafe casts to KernelFunc might not be necessary in all cases if we can declare the function as a KernelFunc in its definition: export const batchToSpaceND: KernelFunc = (.... I haven't tried it with this kernel, but it works for the Real.ts kernel, so I'm guessing it probably generalizes.

LGTM modulo the above nit.

I think we've found in the past that it produces a different set of casts in the kernel func definition itself so went with this convention. I'm not sure what you mean by it works in the Real.ts kernel, i'm looking at that file and not seeing what you are describing.

However one thing your comment did make me notice is that I shouldn't use a const variable for this function. We should prefer named functions where possible and in this case it is quite possible so I'll make that change.

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


tfjs-backend-webgl/src/kernels/BatchToSpaceND.ts, line 79 at r1 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

I think we've found in the past that it produces a different set of casts in the kernel func definition itself so went with this convention. I'm not sure what you mean by it works in the Real.ts kernel, i'm looking at that file and not seeing what you are describing.

However one thing your comment did make me notice is that I shouldn't use a const variable for this function. We should prefer named functions where possible and in this case it is quite possible so I'll make that change.

By "it works in the Real.ts kernel," I meant that, as a local test, I refactored that kernel to conform with my comment. Now that I reread my comment, I realize it reads as if I'm saying Real.ts already conforms to my recommended changes. My bad!

Good point about casting directly to KernelFunc. I forgot we may need to use those functions outside of the KernelConfig, and if they're already cast, we don't know their type.

We can probably remove the casts in the KernelConfig definition, though, since batchToSpaceND (and other kernel implementations) should be a subtype of KernelFunc (I think).

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.

Thank you Yannick. LGTM

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

@tafsiri tafsiri merged commit 24f458b into master Nov 24, 2020
@tafsiri tafsiri deleted the mod-webgl-3 branch November 24, 2020 16:39
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