Skip to content

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

Closed
jcwchen wants to merge 8 commits intomicrosoft:masterfrom
jcwchen:jcw/convtrans-same
Closed

[Dup] Fix SAME_UPPER/SAME_LOWER (auto_pad attribute) in ConvTranspose#9740
jcwchen wants to merge 8 commits intomicrosoft:masterfrom
jcwchen:jcw/convtrans-same

Conversation

@jcwchen
Copy link
Contributor

@jcwchen jcwchen commented Nov 12, 2021 •
edited
Loading

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
ConvolutionTranspose padding mode SAME_UPPER/SAME_LOWER problem #9370
Closed
@jcwchen jcwchen marked this pull request as ready for review November 15, 2021 23:05
jcwchen
jcwchen commented Nov 18, 2021
View reviewed changes
csharp/test/Microsoft.ML.OnnxRuntime.Tests.NetCoreApp/InferenceTest.netcore.cs
{ "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 •
edited
Loading

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
@hariharans29 hariharans29 mentioned this pull request Jun 22, 2022
Closed
@jcwchen
Copy link
Contributor Author

jcwchen commented Jun 23, 2022 •
edited
Loading

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

@jcwchen jcwchen requested a review from hariharans29 June 23, 2022 21:09
@jcwchen jcwchen mentioned this pull request Jun 23, 2022
Closed
@hariharans29
Copy link
Member

hariharans29 commented Jun 24, 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.

IMO should be a "bug fix" rather than "breaking change" - so I think it should be okay. Any conerns @pranavsharma ?

@pranavsharma
Copy link
Contributor

pranavsharma commented Jun 24, 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.

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 jcwchen mentioned this pull request Jun 24, 2022
Merged
@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

Reviewers

@hariharans29 hariharans29 Awaiting requested review from hariharans29

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ConvolutionTranspose padding mode SAME_UPPER/SAME_LOWER problem

3 participants

@jcwchen @hariharans29 @pranavsharma