Align AvgPool ceil_mode on last value to torch #16752
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
titaiwangms
added
converter
justinchuby
reviewed
Jul 18, 2023
justinchuby
reviewed
Jul 18, 2023
| @@ -406,6 +406,7 @@ struct AveragePool1DTask final { | |||
| for (int64_t ph = 0; ph < pooled_height; ++ph) { | |||
| int64_t hstart = ph * stride_h - pads[0]; | |||
| int64_t hend = hstart + kernel_shape[0] * dilation_h; | |||
| hend = std::min(hend, height + pads[1]); | |||
There was a problem hiding this comment.
Is the same needed for other providers as well?
There was a problem hiding this comment.
I only have cpu on my machine, follow #15597, I would only update this on CPU. I am thinking riase another issue to ask for help from core runtime team to apply this to all versions and providers.
There was a problem hiding this comment.
Is the (previous) behavior uniform in all providers? Anyone tested?
justinchuby
previously approved these changes
Jul 18, 2023
titaiwangms
changed the title
titaiwangms
changed the title
liqunfu
reviewed
Jul 19, 2023
titaiwangms
added a commit
to microsoft/onnxscript
that referenced
this pull request
Jul 19, 2023
1D and 3D [Fix](microsoft/onnxruntime#16752) can save the op after opset 19, but not before it. So an xfail is created when ceil_mode=True --------- Co-authored-by: Justin Chu <[email protected]>
xadupre
reviewed
Jul 19, 2023
|
Was this discrepancy clarified by the ONNX spec? Was it due to an implementation oversight of the spec? Hopefully the spec itself was not different than torch? |
This was referenced Jul 20, 2023
titaiwangms
added a commit
to pytorch/pytorch
that referenced
this pull request
Jul 20, 2023
… shapes" In #87892, to pick up the corner cases found in #71549, the PR falls back the implementation of AvgPool to the way opset 9 implementing. However, it introduces a regression on dynamic shape cases found in #101397. This PR refactors the AvgPool op with the same implementation we have in onnxscript: microsoft/onnxscript#754. However, the corner case with `ceil_mode` remains unsolved in onnxruntime: microsoft/onnxruntime#16203. The calculuation on the last value of each dimension is different between ORT and PyTorch. But the fix can be proved in: microsoft/onnxruntime#16752, and it supports AvgPool since opset19. [ghstack-poisoned]
titaiwangms
added a commit
to pytorch/pytorch
that referenced
this pull request
Jul 20, 2023
In #87892, to pick up the corner cases found in #71549, the PR falls back the implementation of AvgPool to the way opset 9 implementing. However, it introduces a regression on dynamic shape cases found in #101397. This PR refactors the AvgPool op with the same implementation we have in onnxscript: microsoft/onnxscript#754. However, the corner case with `ceil_mode` remains unsolved in onnxruntime: microsoft/onnxruntime#16203. The calculuation on the last value of each dimension is different between ORT and PyTorch. But the fix can be proved in: microsoft/onnxruntime#16752, and it supports AvgPool since opset19. [ghstack-poisoned]
titaiwangms
added a commit
to pytorch/pytorch
that referenced
this pull request
Jul 20, 2023
… shapes" In #87892, to pick up the corner cases found in #71549, the PR falls back the implementation of AvgPool to the way opset 9 implementing. However, it introduces a regression on dynamic shape cases found in #101397. This PR refactors the AvgPool op with the same implementation we have in onnxscript: microsoft/onnxscript#754. However, the corner case with `ceil_mode` remains unsolved in onnxruntime: microsoft/onnxruntime#16203. The calculuation on the last value of each dimension is different between ORT and PyTorch. But the fix can be proved in: microsoft/onnxruntime#16752, and it supports AvgPool since opset19. [ghstack-poisoned]
titaiwangms
added a commit
to pytorch/pytorch
that referenced
this pull request
Jul 20, 2023
In #87892, to pick up the corner cases found in #71549, the PR falls back the implementation of AvgPool to the way opset 9 implementing. However, it introduces a regression on dynamic shape cases found in #101397. This PR refactors the AvgPool op with the same implementation we have in onnxscript: microsoft/onnxscript#754. However, the corner case with `ceil_mode` remains unsolved in onnxruntime: microsoft/onnxruntime#16203. The calculuation on the last value of each dimension is different between ORT and PyTorch. But the fix can be proved in: microsoft/onnxruntime#16752, and it supports AvgPool since opset19. [ghstack-poisoned]
titaiwangms
added a commit
to pytorch/pytorch
that referenced
this pull request
Jul 21, 2023
… shapes" In #87892, to pick up the corner cases found in #71549, the PR falls back the implementation of AvgPool to the way opset 9 implementing. However, it introduces a regression on dynamic shape cases found in #101397. This PR refactors the AvgPool op with the same implementation we have in onnxscript: microsoft/onnxscript#754. However, the corner case with `ceil_mode` remains unsolved in onnxruntime: microsoft/onnxruntime#16203. The calculuation on the last value of each dimension is different between ORT and PyTorch. But the fix can be proved in: microsoft/onnxruntime#16752, and it supports AvgPool since opset19. [ghstack-poisoned]
titaiwangms
added a commit
to pytorch/pytorch
that referenced
this pull request
Jul 21, 2023
In #87892, to pick up the corner cases found in #71549, the PR falls back the implementation of AvgPool to the way opset 9 implementing. However, it introduces a regression on dynamic shape cases found in #101397. This PR refactors the AvgPool op with the same implementation we have in onnxscript: microsoft/onnxscript#754. However, the corner case with `ceil_mode` remains unsolved in onnxruntime: microsoft/onnxruntime#16203. The calculuation on the last value of each dimension is different between ORT and PyTorch. But the fix can be proved in: microsoft/onnxruntime#16752, and it supports AvgPool since opset19. [ghstack-poisoned]
titaiwangms
added a commit
to pytorch/pytorch
that referenced
this pull request
Jul 21, 2023
… shapes" In #87892, to pick up the corner cases found in #71549, the PR falls back the implementation of AvgPool to the way opset 9 implementing. However, it introduces a regression on dynamic shape cases found in #101397. This PR refactors the AvgPool op with the same implementation we have in onnxscript: microsoft/onnxscript#754. However, the corner case with `ceil_mode` remains unsolved in onnxruntime: microsoft/onnxruntime#16203. The calculuation on the last value of each dimension is different between ORT and PyTorch. But the fix can be proved in: microsoft/onnxruntime#16752, and it supports AvgPool since opset19. [ghstack-poisoned]
titaiwangms
added a commit
to pytorch/pytorch
that referenced
this pull request
Jul 21, 2023
In #87892, to pick up the corner cases found in #71549, the PR falls back the implementation of AvgPool to the way opset 9 implementing. However, it introduces a regression on dynamic shape cases found in #101397. This PR refactors the AvgPool op with the same implementation we have in onnxscript: microsoft/onnxscript#754. However, the corner case with `ceil_mode` remains unsolved in onnxruntime: microsoft/onnxruntime#16203. The calculuation on the last value of each dimension is different between ORT and PyTorch. But the fix can be proved in: microsoft/onnxruntime#16752, and it supports AvgPool since opset19. [ghstack-poisoned]
pytorchmergebot
pushed a commit
to pytorch/pytorch
that referenced
this pull request
Jul 21, 2023
In #87892, to pick up the corner cases found in #71549, the PR falls back the implementation of AvgPool to the way opset 9 implementing. However, it introduces a regression on dynamic shape cases found in #101397. This PR refactors the AvgPool op with the same implementation we have in onnxscript: microsoft/onnxscript#754. However, the corner case with `count_include_pad` remains unsolved in onnxruntime: microsoft/onnxruntime#16203. The calculuation on the last value of each dimension is different between ORT and PyTorch. But the fix can be proved in: microsoft/onnxruntime#16752, and it supports AvgPool since opset19. Pull Request resolved: #105683 Approved by: https://github.com/thiagocrepaldi
justinchuby
approved these changes
Aug 8, 2023
3 tasks
|
Is this PR still relevant? |
|
Hi @baijumeswani Yes. But this PR only works on Pooling op after opset18. Could you help us to add this supports on older versions as well? Basically, |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Partially fix #16203
Previous to this PR, if
ceil_modeis on, the calculation of a value would divide the kernel size, even if remaining pixels is less than the kernel size, which causes the difference in this operator between ORT and torch.However, this fix only applies to the change in #15597, which only supports AvgPool since 19. The older opset version is remain the same, as it's using mlas files.