Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix CPU fusedConv2d (activation) for NCHW format #6400

Merged
merged 7 commits into from
May 12, 2022

Conversation

Linchenn
Copy link
Collaborator

@Linchenn Linchenn commented May 12, 2022

ISSUE #6368
ISSUE #6354

For NCHW format, if fusedConv2d has a 1-D PReLU activation weights, CPU-backend does not apply it to the channel dimension, while currently just applying it to the last dimension of the input.

This PR does:

  1. For CPU-backend, properly reshape PReLU activation weights to align with the NCHW format to compute fusedConv2d
  2. Moves the shape check of PReLU activation weights from WebGL-backend to tfjs-core, because this checking is general for all backends.

I have manually tested the PReLU activation logics in this PR, using the tests from the PR #6373. These tests will be added when the PR #6373 is merged.

@lina128 , FYI, this is the twin PR with the PR #6379.

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


This change is Reviewable

@Linchenn Linchenn requested a review from pyu10055 May 12, 2022 16:30
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.

thank you for fixing the cpu issue, can the webgl NCHW tests be moved to core within this PR?

Reviewable status: 0 of 1 approvals obtained (waiting on @Linchenn)


tfjs-backend-cpu/src/kernels/FusedConv2D.ts line 87 at r1 (raw file):

          backend, result, activation, reshapedAlpha, leakyreluAlpha);
      backend.disposeIntermediateTensorInfo(reshapedAlpha);
      console.log('Tested');

remove the log here.

Copy link
Collaborator Author

@Linchenn Linchenn left a comment

Choose a reason for hiding this comment

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

Before moving the tests to tfjs-core, we have to enable tfjs-core's fusedConv2d-NCHW and ban some backend's fusedConv2d-NCHW. This involves many codes, so we could do it separately in another PR (#6373 is in progress).

Reviewable status: 0 of 1 approvals obtained (waiting on @mattsoulanille and @pyu10055)


tfjs-backend-cpu/src/kernels/FusedConv2D.ts line 87 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

remove the log here.

Done.

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.

LGTM, and moving the tests in a separate PR sounds good.

Reviewed 2 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @pyu10055)

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 all commit messages.
Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @pyu10055)

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.

Reviewable status: :shipit: complete! 2 of 1 approvals obtained

@Linchenn Linchenn merged commit 6e033c4 into tensorflow:master May 12, 2022
@Linchenn Linchenn deleted the cpu-nchw-prelu branch May 12, 2022 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants