fix: transpose 1D channels_first input in TF depthwise_conv to support dilations#22641
fix: transpose 1D channels_first input in TF depthwise_conv to support dilations#22641satishkc7 wants to merge 4 commits intokeras-team:masterfrom
Conversation
tf.nn.depthwise_conv2d does not support channels_first (NCHW) format with dilations on CPU. When data_format='channels_first' and rank=1, transpose the input from (N, C, W) to (N, W, C) before expanding and running the convolution in NHWC format, then transpose the output back. Also adds test cases covering channels_first with dilation_rate > 1 in both the basic shape test and the correctness test. Fixes keras-team#22534
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request addresses a limitation in the TensorFlow backend where 1D depthwise convolutions with dilations were unsupported on CPU when using the channels_first data format. The implementation transposes the data to channels_last for the operation and then reverts it. Feedback suggests using the existing internal helper functions _transpose_spatial_inputs and _transpose_spatial_outputs for these transpositions to ensure consistency with established patterns in the TensorFlow backend.
keras/src/backend/tensorflow/nn.py
Outdated
| if data_format == "channels_first": | ||
| inputs = tf.transpose(inputs, perm=[0, 2, 1]) |
There was a problem hiding this comment.
While this manual transpose works, the file already contains helper functions _transpose_spatial_inputs and _transpose_spatial_outputs (lines 188 and 206) that are used throughout the TensorFlow backend to handle exactly this kind of channels_first limitation. Using them would be more consistent with the rest of the file.
| if data_format == "channels_first": | |
| inputs = tf.transpose(inputs, perm=[0, 2, 1]) | |
| if data_format == "channels_first": | |
| inputs = _transpose_spatial_inputs(inputs) |
keras/src/backend/tensorflow/nn.py
Outdated
| if data_format == "channels_first": | ||
| outputs = tf.transpose(outputs, perm=[0, 2, 1]) |
There was a problem hiding this comment.
As with the input transpose, using the existing helper function _transpose_spatial_outputs would be more consistent with the repository's patterns.
| if data_format == "channels_first": | |
| outputs = tf.transpose(outputs, perm=[0, 2, 1]) | |
| if data_format == "channels_first": | |
| outputs = _transpose_spatial_outputs(outputs) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #22641 +/- ##
=======================================
Coverage 83.25% 83.26%
=======================================
Files 596 596
Lines 68244 68246 +2
Branches 10667 10668 +1
=======================================
+ Hits 56816 56823 +7
+ Misses 8638 8635 -3
+ Partials 2790 2788 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
OpenVINO does not support channels_first for depthwise_conv and raises ValueError. Skip the new channels_first + dilation test cases when running on the OpenVINO backend.
hertschuh
left a comment
There was a problem hiding this comment.
Thanks for working on this fix!
| if data_format == "channels_first" and backend.backend() == "openvino": | ||
| self.skipTest( | ||
| "OpenVINO backend does not support channels_first for " | ||
| "depthwise_conv." | ||
| ) |
There was a problem hiding this comment.
Can you add the test name to this list instead?
https://github.com/keras-team/keras/blob/master/keras/src/backend/openvino/excluded_concrete_tests.txt
This is how we track feature gaps in OpenVino
| if data_format == "channels_first" and backend.backend() == "openvino": | ||
| self.skipTest( | ||
| "OpenVINO backend does not support channels_first for " | ||
| "depthwise_conv." | ||
| ) |
There was a problem hiding this comment.
Can you add the test name to this list instead?
https://github.com/keras-team/keras/blob/master/keras/src/backend/openvino/excluded_concrete_tests.txt
This is how we track feature gaps in OpenVino
| # `tf.nn.depthwise_conv2d` does not support `channels_first` with | ||
| # dilations on CPU. Transpose to `channels_last`, compute, and | ||
| # transpose back to avoid the limitation. | ||
| if data_format == "channels_first": | ||
| inputs = _transpose_spatial_inputs(inputs) |
There was a problem hiding this comment.
Can we add a check to only do this on CPU? I would assume transposing is slower than the current GPU / TPU implementations that currently work with "channels_first".
Summary
tf.nn.depthwise_conv2ddoes not supportchannels_first(NCHW) format with dilations on CPU. WhenDepthwiseConv1Dis used withdata_format='channels_first'anddilation_rate > 1, the TF backend'sdepthwise_convfunction passed NCHW data directly totf.nn.depthwise_conv2d, which fails at runtime with anIncompatible shapeserror.Root Cause
In
keras/src/backend/tensorflow/nn.py, the 1D depthwise conv path forchannels_firstsetspatial_start_dim = 2and expanded the input as(N, C, 1, W)in NCHW format. Passing NCHW + dilations totf.nn.depthwise_conv2dis not supported on CPU, causing the op to return incorrect results and ultimately anIncompatible shapeserror when adding the bias.Fix
For the 1D
channels_firstcase, use the existing_transpose_spatial_inputsand_transpose_spatial_outputshelpers to transpose tochannels_lastbefore the convolution and back after, consistent with the pattern used bymax_poolandaverage_poolin the same file.Changes
keras/src/backend/tensorflow/nn.py: use_transpose_spatial_inputs/outputshelpers for 1D channels_first depthwise conv, compute in NHWC formatkeras/src/layers/convolutional/depthwise_conv_test.py: addchannels_first+dilation_rate=(2,)test cases to both the basic shape test and the correctness testFixes #22534
Contributor Agreement