[Fix] keras.ops.separable_conv rejects valid symbolic inputs with dilated kernels#22566
[Fix] keras.ops.separable_conv rejects valid symbolic inputs with dilated kernels#22566ChiragSW wants to merge 5 commits intokeras-team:masterfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces logic to handle kernels with an optional batch dimension in compute_output_spec by stripping the leading dimension when it matches the input rank plus one. Additionally, it modifies compute_conv_output_shape in operation_utils.py to set non-positive spatial dimensions to zero instead of raising a ValueError. The review feedback suggests extracting the duplicated kernel shape adjustment logic into a shared utility function to improve maintainability and reduce code duplication.
keras/src/ops/nn.py
Outdated
| kernel_shape = kernel.shape | ||
| if len(kernel_shape) == len(inputs.shape) + 1 and kernel_shape[0] in ( | ||
| None, | ||
| 1, | ||
| ): | ||
| kernel_shape = kernel_shape[1:] |
There was a problem hiding this comment.
This logic to handle a potential batch dimension on the kernel is duplicated in DepthwiseConv.compute_output_spec (lines 1560-1565). To improve maintainability and avoid code duplication, consider extracting this logic into a helper function in keras/src/ops/operation_utils.py.
For example, you could add a function like this to operation_utils.py:
def unexpand_kernel_shape(kernel_shape, input_rank):
"""Removes a leading batch dimension from kernel_shape if present."""
if len(kernel_shape) == input_rank + 1 and kernel_shape[0] in (None, 1):
return kernel_shape[1:]
return kernel_shapeThen, you could simplify this method and DepthwiseConv.compute_output_spec by calling this new helper function.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #22566 +/- ##
==========================================
+ Coverage 83.26% 83.28% +0.01%
==========================================
Files 596 596
Lines 67828 68096 +268
Branches 10562 10608 +46
==========================================
+ Hits 56480 56716 +236
- Misses 8605 8635 +30
- Partials 2743 2745 +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:
|
|
@hertschuh please review the PR |
keerthanakadiri
added
the
stat:awaiting keras-eng
hertschuh
left a comment
hertschuh
left a comment
There was a problem hiding this comment.
Thanks for looking into this!
| if self.padding == "valid": | ||
| if self.data_format == "channels_last": | ||
| spatial_dims = output_shape[1:-1] | ||
| else: | ||
| spatial_dims = output_shape[2:] | ||
| if any(d == 0 for d in spatial_dims if d is not None): | ||
| raise ValueError( | ||
| "Computed output size would be negative. Received: " | ||
| f"`inputs.shape={input_shape}`, " | ||
| f"`kernel_size={self.kernel_size}`, `dilation_rate=" | ||
| f"{self.dilation_rate}`, `strides={self.strides}`." | ||
| ) | ||
| return output_shape |
There was a problem hiding this comment.
Please move this code inside compute_conv_output_shape so that all ops can benefit from this check.
keras/src/ops/operation_utils.py
Outdated
| raise ValueError( | ||
| "Computed output size would be zero or negative. Received " | ||
| f"`inputs shape={input_shape}`, " | ||
| f"`kernel shape={kernel_shape}`, " | ||
| f"`dilation_rate={dilation_rate}`, " | ||
| f"`strides={strides}`, " | ||
| f"`padding={padding}`." | ||
| ) | ||
| output_spatial_shape[i] = 0 |
There was a problem hiding this comment.
Replace with the more correct test that you added in keras/src/layers/convolutional/base_conv.py.
And don't return 0 of the dimension is computed as negative.
keras/src/ops/operation_utils.py
Outdated
| def unexpand_kernel_shape(kernel_shape, input_rank): | ||
| """Removes a leading batch dimension from `kernel_shape` if present.""" | ||
| if len(kernel_shape) == input_rank + 1 and kernel_shape[0] in (None, 1): | ||
| return kernel_shape[1:] | ||
| return kernel_shape |
There was a problem hiding this comment.
This part does not make sense to me. The kernel shape is unrelated to whether the batch dimension is present in the inputs or not. There is no "kernel batch dimension".
There was a problem hiding this comment.
In the new commit, I have added the description for this in docstring
hertschuh
added
stat:awaiting response from contributor
and removed
stat:awaiting keras-eng
5 participants
Fixes issue: #22517
Root cause
keras.Input(shape=...) adds a batch dim, so your kernels become rank-5 like (None, 3, 3, 3, 1) instead of rank-4.
Symbolic shape inference treated that leading None as part of the kernel shape, triggering the kernel-rank mismatch error.
Fix
In keras/src/ops/nn.py, symbolic Conv/DepthwiseConv shape inference now ignores a leading (None|1) dim from kernels before computing output spec.
In keras/src/ops/operation_utils.py, compute_conv_output_shape now clamps negative/zero VALID outputs to 0 (instead of raising) for consistent symbolic behavior.