Skip to content

Conversation

@annxingyuan
Copy link
Contributor

@annxingyuan annxingyuan commented Jun 10, 2020

This fixes #3311

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


This change is Reviewable

@annxingyuan annxingyuan requested a review from pyu10055 June 10, 2020 11:51
@annxingyuan annxingyuan self-assigned this Jun 10, 2020
@annxingyuan annxingyuan requested review from lina128 and tafsiri and removed request for lina128 and tafsiri June 10, 2020 18:11
@annxingyuan annxingyuan marked this pull request as draft June 10, 2020 18:20
@annxingyuan annxingyuan marked this pull request as ready for review June 10, 2020 18:48
Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

Reviewed 12 of 12 files at r1.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @annxingyuan and @pyu10055)


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

type PadType = 'SAME'|'VALID'|'NUMBER'|'EXPLICIT';

// For NHWC should be in the following form:

Is the case that we only support NHWC format not the NCHW?

Copy link
Contributor Author

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

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @pyu10055)


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

Previously, pyu10055 (Ping Yu) wrote…

Is the case that we only support NHWC format not the NCHW?

Ah, thanks for catching that. I looked at the validation logic for conv1d and assumed we only allowed NHWC, but in conv2d we allow NCHW. I've added support for NCHW explicit padding.

@annxingyuan annxingyuan merged commit 9b50644 into master Jun 12, 2020
@annxingyuan annxingyuan deleted the conv_explicit_pad branch June 12, 2020 12:13
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.

Unknown padding parameter: explicit

3 participants