Conversation
The pool converter materialized every input dimension as a Python int to derive SAME_UPPER/SAME_LOWER padding, so MaxPool/AveragePool/LpPool failed with `int(Var)` on graphs with symbolic spatial extents (e.g. PP-OCR's dynamic-width MaxPool). With unit stride the SAME padding is `dilated_kernel - 1` regardless of the input extent, so compute it without touching the shape and only require a static extent when stride > 1, raising OpAttributeUnImplemented otherwise. Unifying the max/avg paths into one spec-following helper also fixes two silent correctness issues: MaxPool SAME_LOWER used floor(in / stride) instead of ceil for the output extent, producing incorrect pads when the extent is not divisible by the stride, and AveragePool ignored dilations when resolving auto_pad. Fixes apache#20398
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The ONNX pool converter converted every input dimension to a Python
intin order to resolveauto_pad=SAME_UPPER/SAME_LOWER, soMaxPool,AveragePoolandLpPoolfailed withTypeError: int() argument must be ... not 'Var'whenever a spatial extent was symbolic. #20398 reports this for a dynamic-widthMaxPooltaken from PP-OCR.The input extent is not actually needed in the reported case. Both SAME modes produce
ceil(input / stride)outputs, so the total padding is(ceil(in / s) - 1) * s + dilated_kernel - in, which reduces todilated_kernel - 1when the stride is 1. This PR computes the padding from that formula in a single helper,Pool.get_same_pads, which only reads an extent when the stride is greater than 1. A symbolic extent combined with a non-unit stride still cannot be expressed as a static padding attribute, so it now raisesOpAttributeUnImplementedwith a message naming the dimension, instead of theTypeError.Replacing the separate max-pool and avg-pool code paths with one spec-following helper also corrects two silent issues.
MaxPoolwithSAME_LOWERderived the output extent withfloor(in / stride)where the spec requiresceil, which gave wrong padding whenever the extent was not divisible by the stride.AveragePoolignoreddilationswhen resolvingauto_pad.Tests compare against onnxruntime for extents that are not divisible by the stride, and cover symbolic batch and width dimensions for all three ops (structurally, and numerically with a concrete input). The dilated case is checked structurally against the output shape given by ONNX shape inference, because onnxruntime ignores dilation when resolving
auto_padand returns a smaller output than the model's inferred shape. When the kernel is smaller than the stride the spec formula yields negative padding; the helper clamps it to zero, and this path has no onnxruntime comparison because onnxruntime rejects such models.Fixes #20398