-
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
Fix RoiAlign CPU EP issues #7354
base: main
Are you sure you want to change the base?
Conversation
…e valid and should be treated as points (3) max mode should take the entire bilinear sample, not a fragment of it
Thanks for referring my PR to this PR. I think we agree that the ultimate solution is to update onnx spec for ROIAlign - we need add an attribute |
@hariharans29 - do you have instructions on updating this C# test baseline? I believe these values were recorded with the bug baked into them:
|
Maybe we could change the test referencing the PR and stating that the expected values were generated with the buggy implementation to begin with ? |
7cbe5ec
to
aa5461d
Compare
Is there an issue filed with ONNX? We need to reference it here. |
Well here was the original ONNX issue onnx/onnx#3428, which RoiAlign opset 16 addressed by adding |
### Description <!-- Describe your changes. --> Add registration for DML RoiAlign-16 and tests for new coordinate_transform_mode attribute. PR [7354](#7354) is still open to fix the CPU EP version, which is why there are skipped tests right now. That will be completed separately so that, for now, we can officially support opset16 with the next release. ### Motivation and Context <!-- - Why is this change required? What problem does it solve? - If it fixes an open issue, please link to the issue here. --> --------- Co-authored-by: Linnea May <[email protected]> Co-authored-by: Dwayne Robinson <[email protected]>
### Description <!-- Describe your changes. --> Add registration for DML RoiAlign-16 and tests for new coordinate_transform_mode attribute. PR [7354](#7354) is still open to fix the CPU EP version, which is why there are skipped tests right now. That will be completed separately so that, for now, we can officially support opset16 with the next release. ### Motivation and Context <!-- - Why is this change required? What problem does it solve? - If it fixes an open issue, please link to the issue here. --> --------- Co-authored-by: Linnea May <[email protected]> Co-authored-by: Dwayne Robinson <[email protected]>
### Description <!-- Describe your changes. --> Add registration for DML RoiAlign-16 and tests for new coordinate_transform_mode attribute. PR [7354](#7354) is still open to fix the CPU EP version, which is why there are skipped tests right now. That will be completed separately so that, for now, we can officially support opset16 with the next release. ### Motivation and Context <!-- - Why is this change required? What problem does it solve? - If it fixes an open issue, please link to the issue here. --> --------- ### Description <!-- Describe your changes. --> ### Motivation and Context <!-- - Why is this change required? What problem does it solve? - If it fixes an open issue, please link to the issue here. --> Co-authored-by: Linnea May <[email protected]> Co-authored-by: Dwayne Robinson <[email protected]>
### Description <!-- Describe your changes. --> Add registration for DML RoiAlign-16 and tests for new coordinate_transform_mode attribute. PR [7354](#7354) is still open to fix the CPU EP version, which is why there are skipped tests right now. That will be completed separately so that, for now, we can officially support opset16 with the next release. ### Motivation and Context <!-- - Why is this change required? What problem does it solve? - If it fixes an open issue, please link to the issue here. --> --------- Co-authored-by: Linnea May <[email protected]> Co-authored-by: Dwayne Robinson <[email protected]>
### Description <!-- Describe your changes. --> Add registration for DML RoiAlign-16 and tests for new coordinate_transform_mode attribute. PR [7354](microsoft#7354) is still open to fix the CPU EP version, which is why there are skipped tests right now. That will be completed separately so that, for now, we can officially support opset16 with the next release. ### Motivation and Context <!-- - Why is this change required? What problem does it solve? - If it fixes an open issue, please link to the issue here. --> --------- ### Description <!-- Describe your changes. --> ### Motivation and Context <!-- - Why is this change required? What problem does it solve? - If it fixes an open issue, please link to the issue here. --> Co-authored-by: Linnea May <[email protected]> Co-authored-by: Dwayne Robinson <[email protected]>
Description:
(1) max mode should take the entire bilinear sample, not a fragment of it. This line should be
output value = max(sum of all weighted values, previous maximum)
, notoutput value = max(max(max(max(value1, value2), value3) value4), previous maximum)
.(2) 0 size regions are valid and should be treated as sampled points, not forced to 1. This leads to test failures when sampling tiny points or slivers in our ONNX conformance tests.
bin_size_w
andbin_size_h
are only multiplied anyway, not divided, and so an attempt to prevent division by zero (or whatever the original intention) is unnecessary.(3) The ONNX compat attribute was already added in onnx/onnx#3625
input pixel alignment is 0.5 off - the correct equation is(x + 0.5) * scale - 0.5
, but the final 0.5 was missed(x + 0.5) * scale
(see https://arxiv.org/abs/1703.06870).Motivation and Context