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

[Dup] Fix SAME_UPPER/SAME_LOWER (auto_pad attribute) in ConvTranspose #9740

Closed
wants to merge 8 commits into from

Conversation

jcwchen
Copy link
Contributor

@jcwchen jcwchen commented Nov 12, 2021

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:

if auto_pads == 'SAME_UPPER':
  pad_head = paddings / 2  # smaller one 
  pad_tail = paddings - paddings / 2  # larger one
elif auto_pads == 'SAME_LOWER':
  pad_head = paddings - paddings / 2  # larger one
  pad_tail = paddings / 2  # smaller one

Motivation and Context
The auto_pad attribute, SAME_UPPER and SAME_LOWER of ConvTranspose 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.

@jcwchen jcwchen marked this pull request as draft November 12, 2021 01:16
@jcwchen jcwchen changed the title Fix SAME_UPPER/SAME_LOWER (auto_pad attribute) in ConvTranspose [Dup] Fix SAME_UPPER/SAME_LOWER (auto_pad attribute) in ConvTranspose Nov 12, 2021
@jcwchen jcwchen linked an issue Nov 12, 2021 that may be closed by this pull request
@jcwchen jcwchen marked this pull request as ready for review November 15, 2021 23:05
@@ -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" },
Copy link
Contributor Author

@jcwchen jcwchen Nov 18, 2021

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

@stale
Copy link

stale bot commented Apr 16, 2022

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.

@stale stale bot added the stale issues that have not been addressed in a while; categorized by a bot label Apr 16, 2022
@jcwchen
Copy link
Contributor Author

jcwchen commented Apr 16, 2022

Please note that this issue still exists.

@stale stale bot removed the stale issues that have not been addressed in a while; categorized by a bot label Apr 16, 2022
@jcwchen
Copy link
Contributor Author

jcwchen commented Jun 23, 2022

@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.

@hariharans29
Copy link
Member

@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 ?

@pranavsharma
Copy link
Contributor

@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

  1. print a warning for the upcoming 1.12 release
  2. mention this in the release notes under the "Upcoming backward incompatible bug fixes" section
  3. actually fix the bug in 1.13 release?

@jcwchen
Copy link
Contributor Author

jcwchen commented Jun 24, 2022

Thank you both. @pranavsharma your plan looks good to me. I have proposed another PR to print warning for upcoming 1.12 first: #11984.

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.

ConvolutionTranspose padding mode SAME_UPPER/SAME_LOWER problem
3 participants