Skip to content

Fix fuse_pad_into_pool folding zero-padding into MaxPool - #318

Open
take-cheeze wants to merge 2 commits into
onnx:mainfrom
take-cheeze:claude/onnxsim-issue-290-2jtehp
Open

take-cheeze wants to merge 2 commits into
onnx:mainfrom
take-cheeze:claude/onnxsim-issue-290-2jtehp

Conversation

@take-cheeze

Copy link
Copy Markdown
Member

MaxPool ignores its padded elements, which is equivalent to padding with -inf. The pass previously folded a Pad with constant_value=0 (or an unspecified value, which defaults to 0) into MaxPool by moving the padding into the pool's pads attribute. This changes the result whenever a pooling window's real values are all negative: Pad(0)+MaxPool yields 0 while the fused MaxPool yields the (negative) window maximum.

Make the required Pad constant value depend on the pool type:

  • AveragePool (with count_include_pad=1): 0
  • MaxPool: -inf

Also restructure the constant-value check so an unspecified Pad value correctly blocks fusion into MaxPool instead of being treated as a match.

Update the MaxPool tests that encoded the old behavior to assert no fusion for value=0/default, and add companion tests confirming Pad(value=-inf) folds correctly.

Fixes onnxsim/onnxsim#290

Claude-Session: https://claude.ai/code/session_013sB8xKJyd86p47c2vfXMVD

@take-cheeze
take-cheeze requested review from a team as code owners July 24, 2026 08:56
MaxPool ignores its padded elements, which is equivalent to padding with
-inf. The pass previously folded a Pad with constant_value=0 (or an
unspecified value, which defaults to 0) into MaxPool by moving the padding
into the pool's `pads` attribute. This changes the result whenever a
pooling window's real values are all negative: Pad(0)+MaxPool yields 0
while the fused MaxPool yields the (negative) window maximum.

Make the required Pad constant value depend on the pool type:
  - AveragePool (with count_include_pad=1): 0
  - MaxPool:                                -inf

Also restructure the constant-value check so an unspecified Pad value
correctly blocks fusion into MaxPool instead of being treated as a match.

Update the MaxPool tests that encoded the old behavior to assert no fusion
for value=0/default, and add companion tests confirming Pad(value=-inf)
folds correctly.

Fixes onnxsim/onnxsim#290

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013sB8xKJyd86p47c2vfXMVD
Signed-off-by: take-cheeze <takechi101010@gmail.com>
Address correctness findings in onnx#318 and add targeted regression coverage. Native builds and focused tests were verified locally on Windows.

Signed-off-by: Andreas Fehlner <fehlner@arcor.de>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@andife

andife commented Oct 18, 2026

Copy link
Copy Markdown
Member

Added a signed-off follow-up fix in a79a41c, preserving this PR's original contribution and history.

Preserve observable MaxPool indices: skip Pad fusion when the indices output is consumed or exported, while still allowing an unused second output.

Validation on Windows after rebuilding the native extension against this branch's pinned ONNX:

  • python -m pytest -o addopts= onnxoptimizer\test\optimizer_test.py -k "fuse_pad_into_maxpool or fuse_pad_into_avgpool" -q — 35 passed; 3 subtests passed.
  • git diff --check — passed.

Full suite and non-Windows builds were not run locally. This follow-up was prepared with LLM assistance and checked with targeted regression tests (see #357).

@andife andife added the bug Something isn't working label Oct 18, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] fold pad(constant_value=0) into maxpool

3 participants