Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ONNX] Fix interpreting auto_pad parameters in ConvTranspose operator #16001

Merged
merged 1 commit into from
Apr 8, 2024

Conversation

padreofthegame
Copy link
Contributor

@padreofthegame padreofthegame commented Oct 27, 2023

Fix in interpreting auto_pad parameters SAME_UPPER and SAME_LOWER in ConvTranspose version 1 and 11 operator according to documentation on link.

Tests that were failing are also added in test_forward.py.

Also, enable unsupported test for ONNX: test_convtranspose_autopad_same.

@padreofthegame
Copy link
Contributor Author

@tvm-bot rerun

@padreofthegame padreofthegame force-pushed the onnx_pad_upper_convtranspose branch from 378f598 to 89373d8 Compare October 31, 2023 17:00
@padreofthegame padreofthegame changed the title [WIP][ONNX] Fix interpreting auto_pad parameters in ConvTranspose operator [ONNX] Fix interpreting auto_pad parameters in ConvTranspose operator Oct 31, 2023
@padreofthegame
Copy link
Contributor Author

Hello,

looks like that the problem with this PRs failure is due to the different implementations of convTranspose operator in different versions of onnxruntime. Precissely, onnxruntime 1.12 and 1.15 gives different results when the parameters SAME_UPPER and SAME_LOWER are used. On my local machine, this tests passes when onnxruntime 1.15 is used, and fail with 1.12 (probably due to wrong implementation), although the operator is implemented according to ONNX documentation.

What could be the next step for this PR? What is the common practice here:
- to skip this test, as it was done with test_convtranspose_autopad_same?
- to mark this test as flaky?
- to add part of the code to check onnxruntime version before test running?

Sorry if I tagged someone not related to the topic.

@masahi @Hzfengsy @jikechao

@padreofthegame padreofthegame force-pushed the onnx_pad_upper_convtranspose branch from 89373d8 to d63098a Compare November 26, 2023 17:38
@padreofthegame
Copy link
Contributor Author

Hello,

looks like this PR is staying opened this way for quite some time now. Any feedback related to this PR?

@padreofthegame padreofthegame force-pushed the onnx_pad_upper_convtranspose branch from d63098a to 9de8714 Compare March 16, 2024 18:03
@padreofthegame
Copy link
Contributor Author

Hello,

Any updates on this one? Can we get this merged as it has been approved for quite some time now, and is passing all the tests?

@yongwww yongwww merged commit 0594994 into apache:main Apr 8, 2024
18 checks passed
@yongwww
Copy link
Member

yongwww commented Apr 8, 2024

@padreofthegame it was merged! Thanks for the fix!

@padreofthegame padreofthegame deleted the onnx_pad_upper_convtranspose branch April 9, 2024 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants