-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[Dup] Fix SAME_UPPER/SAME_LOWER (auto_pad attribute) in ConvTranspose #9740
Conversation
a15f311
to
3e0ce22
Compare
@@ -164,6 +164,7 @@ private void TestTensorRTProviderOptions() | |||
{ "tf_resnet_v1_50", "result mismatch when Conv BN Fusion is applied" }, | |||
{ "tf_resnet_v1_101", "result mismatch when Conv BN Fusion is applied" }, | |||
{ "tf_resnet_v1_152", "result mismatch when Conv BN Fusion is applied" }, | |||
{ "cntk_simple_seg", "Bad onnx test output caused by wrong SAME_UPPER/SAME_LOWER for ConvTranspose" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Globally remove cntk_simple_seg
for now because it produces wrong output with old implementation. I will add it back after updating the output in another PR
This issue has been automatically marked as stale due to inactivity and will be closed in 7 days if no further activity occurs. If further support is needed, please provide an update and/or more details. |
Please note that this issue still exists. |
@hariharans29 do you have time to review this PR? I just rebased this old PR with the latest branch. There are two issues about it already: 9370, 11927. Since it's a computation behavior change in ORT, do you have any concern regarding backward compatibility? Thank you. |
IMO should be a "bug fix" rather than "breaking change" - so I think it should be okay. Any conerns @pranavsharma ? |
I can't really say how many models deployed in production would break because of this fix. Can we
|
Thank you both. @pranavsharma your plan looks good to me. I have proposed another PR to print warning for upcoming 1.12 first: #11984. |
Description:
Dup of #5368. Recreate this PR for cleaner commit log. Target it to ORT 1.10
To sync the definition of SAME_UPPER/SAME_LOWER among all operators and make it same as ONNX definition, switch the logic of SAME_UPPER and SAME_LOWER in ConvTranspose.
Definition of SAME_UPPER and SAME_LOWER should be as follows:
Motivation and Context
The
auto_pad
attribute,SAME_UPPER
andSAME_LOWER
ofConvTranspose
is different from other operators' (pool and conv related operators)auto_pad
attribute. The behavior of same attribute should be the same among all operators. Also, it does not meet the definition in ONNX.SAME_UPPER
andSAME_LOWER
in other operatorsonnxruntime/onnxruntime/core/providers/cpu/nn/pool_attributes.h
Line 149 in c20fcf2
https://github.com/onnx/onnx/blob/b2ed660d0a065b8346816f2c3a95d79ca79b88c9/onnx/defs/nn/defs.cc#L1222
Update spec for Convtranspose to make it sync onnx/onnx#3019