Skip to content

Conversation

@annxingyuan
Copy link
Contributor

@annxingyuan annxingyuan commented Sep 5, 2019

Changes

  • Add new fusedDepthwiseConv2D method to KernelBackend that fuses 2d depthwise convolutions with bias and activation.

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


This change is Reviewable

@annxingyuan annxingyuan self-assigned this Sep 5, 2019
@annxingyuan annxingyuan requested a review from pyu10055 September 5, 2019 15:30
Copy link
Contributor

@nsthorat nsthorat left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 8 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @nsthorat, and @pyu10055)


tfjs-core/src/backends/webgl/backend_webgl.ts, line 2128 at r1 (raw file):

  fusedDepthwiseConv2D(
      x: Tensor4D, filter: Tensor4D, convInfo: Conv2DInfo, bias?: Tensor4D,

should we also make this an object like we do for fusedBatchedMatMul?


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

export const conv2dDerInput = op({conv2dDerInput_});
export const depthwiseConv2d = op({depthwiseConv2d_});
export const depthwiseConv2dDerInput = op({depthwiseConv2dDerInput_});

why do we need to export these?


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

/**
 * Depthwise 2D convolution.

Add messaging here about fusing

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: 0 of 1 approvals obtained (waiting on @dsmilkov, @nsthorat, and @pyu10055)


tfjs-core/src/backends/webgl/backend_webgl.ts, line 2128 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

should we also make this an object like we do for fusedBatchedMatMul?

Done


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

Previously, nsthorat (Nikhil Thorat) wrote…

why do we need to export these?

They're needed to compute gradients.


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

Previously, nsthorat (Nikhil Thorat) wrote…

Add messaging here about fusing

Done

Copy link
Contributor

@nsthorat nsthorat 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, @dsmilkov, and @pyu10055)


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

Previously, annxingyuan (Ann Yuan) wrote…

They're needed to compute gradients.

Is someone doing this outside the library?

If not let's remove this since it will be part of the public API

If so -- then you should also make the gradient calls above use the public op method rather than the private one with the underscore

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 @dsmilkov and @pyu10055)


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

Previously, nsthorat (Nikhil Thorat) wrote…

Is someone doing this outside the library?

If not let's remove this since it will be part of the public API

If so -- then you should also make the gradient calls above use the public op method rather than the private one with the underscore

Done

@annxingyuan annxingyuan merged commit 61b355e into master Sep 6, 2019
@annxingyuan annxingyuan deleted the fused_depthwise branch September 6, 2019 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants