Fix Torch output_padding constraint for ConvTranspose layers #21852
Conversation
Summary of ChangesHello @MalyalaKarthik66, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical compatibility issue with the Torch backend when handling Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 addresses a runtime error in the Torch backend for ConvTranspose layers where output_padding could be greater than or equal to stride. The fix replaces a ValueError with a warning and clamps the output_padding value, which is a good approach to improve user experience and backend consistency. A new test case validates this behavior. My review includes a suggestion to consolidate the clamping logic into a single place for better maintainability and to make it more robust against invalid stride values, which involves removing a redundant block of code.
| warnings.warn( | ||
| f"Torch backend requires output_padding < stride. " | ||
| f"Clamping output_padding {torch_output_padding} -> {stride - 1} " | ||
| f"for stride {stride}.", | ||
| UserWarning, | ||
| ) | ||
| torch_output_padding = stride - 1 |
There was a problem hiding this comment.
This clamping logic is a good improvement. However, the redundant clamping logic later in compute_conv_transpose_padding_args_for_torch (lines 187-201) uses max(0, s - 1), which is safer as it prevents negative padding if stride is less than 1. It would be best to incorporate that safer logic here and remove the redundant code block. Using a temporary variable for the new padding value would also improve the warning message's clarity.
| warnings.warn( | |
| f"Torch backend requires output_padding < stride. " | |
| f"Clamping output_padding {torch_output_padding} -> {stride - 1} " | |
| f"for stride {stride}.", | |
| UserWarning, | |
| ) | |
| torch_output_padding = stride - 1 | |
| new_output_padding = max(0, stride - 1) | |
| warnings.warn( | |
| f"Torch backend requires output_padding < stride. " | |
| f"Clamping output_padding {torch_output_padding} -> {new_output_padding} " | |
| f"for stride {stride}.", | |
| UserWarning, | |
| ) | |
| torch_output_padding = new_output_padding |
| # --- FIX FOR TORCH CONSTRAINT: output_padding < stride --- | ||
| corrected_output_paddings = [] | ||
| for s, op in zip( | ||
| strides | ||
| if isinstance(strides, (list, tuple)) | ||
| else [strides] * num_spatial_dims, | ||
| torch_output_paddings, | ||
| ): | ||
| max_allowed = max(0, s - 1) | ||
| if op > max_allowed: | ||
| corrected_output_paddings.append(max_allowed) | ||
| else: | ||
| corrected_output_paddings.append(op) | ||
|
|
||
| torch_output_paddings = corrected_output_paddings |
There was a problem hiding this comment.
With the clamping logic now handled within _convert_conv_transpose_padding_args_from_keras_to_torch (and improved with the suggestion in the other comment), this entire block of code becomes redundant. Removing it will simplify the function and avoid logic duplication.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #21852 +/- ##
=======================================
Coverage 82.47% 82.47%
=======================================
Files 577 577
Lines 59508 59516 +8
Branches 9332 9334 +2
=======================================
+ Hits 49080 49088 +8
Misses 8015 8015
Partials 2413 2413
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:
|
3 participants
Fix: #20949
This PR fixes the Torch backend error where output_padding >= stride results in invalid shapes for ConvTranspose ops.
Changes
Added clamping logic in _convert_conv_transpose_padding_args_from_keras_to_torch to ensure output_padding < stride (Torch requirement).
Added clear UserWarning when clamping occurs.
Added dedicated test: test_output_padding_clamped_for_torch_constraint.
This ensures consistent behavior across backends and prevents Torch runtime failures.