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

DML EP RoiAlign-16 #15812

Merged
merged 16 commits into from
May 10, 2023
Merged

DML EP RoiAlign-16 #15812

merged 16 commits into from
May 10, 2023

Conversation

linnealovespie
Copy link
Contributor

@linnealovespie linnealovespie commented May 4, 2023

Description

Add registration for DML RoiAlign-16 and tests for new coordinate_transform_mode attribute. PR 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

Formatting self-consistency
Copy link
Contributor

@fdwr fdwr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would validate the coordinate transformation string. Otherwise LGTM.

fdwr
fdwr previously approved these changes May 5, 2023
Copy link
Contributor

@fdwr fdwr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comment typos. Otherwise LGTM.

@@ -267,7 +506,194 @@ TEST(RoiAlignTest, MaxModePositive) {
test.AddInput<float>("rois", {5, 4}, {7., 5., 7., 5., -15., -15., -15., -15., -10., 21., -10., 21., 13., 8., 13., 8., -14., 19., -14., 19.});
test.AddInput<int64_t>("batch_indices", {5}, {0, 0, 0, 0, 0});
test.AddOutput<float>("Y", {5, 3, 3, 4}, {2.10938f, 2.95313f, 3.375f, 2.53125f, 3.35938f, 4.70313f, 5.375f, 4.03125f, 3.51563f, 4.92188f, 5.625f, 4.21875f, 10.8984f, 15.2578f, 17.4375f, 13.0781f, 17.3568f, 24.2995f, 27.7708f, 20.8281f, 18.1641f, 25.4297f, 29.0625f, 21.7969f, 19.6875f, 27.5625f, 31.5f, 23.625f, 31.3542f, 43.8958f, 50.1667f, 37.625f, 32.8125f, 45.9375f, 52.5f, 39.375f, 0.f, 0.f, 0.f, 0.f, 0.f, 0.f, 0.f, 0.f, 0.f, 0.f, 0.f, 0.f, 25.f, 25.f, 25.f, 25.f, 25.f, 25.f, 25.f, 25.f, 25.f, 25.f, 25.f, 25.f, 50.f, 50.f, 50.f, 50.f, 50.f, 50.f, 50.f, 50.f, 50.f, 50.f, 50.f, 50.f, 5.625f, 5.625f, 5.625f, 4.57031f, 8.95833f, 8.95833f, 8.95833f, 7.27865f, 9.375f, 9.375f, 9.375f, 7.61719f, 19.6875f, 19.6875f, 19.6875f, 15.9961f, 31.3542f, 31.3542f, 31.3542f, 25.4753f, 32.8125f, 32.8125f, 32.8125f, 26.6602f, 33.75f, 33.75f, 33.75f, 27.4219f, 53.75f, 53.75f, 53.75f, 43.6719f, 56.25f, 56.25f, 56.25f, 45.7031f, 4.5f, 3.9375f, 2.8125f, 3.9375f, 5.5f, 4.8125f, 3.4375f, 4.8125f, 4.58333f, 4.01042f, 2.86458f, 3.9375f, 23.25f, 20.3438f, 14.5313f, 18.f, 28.4167f, 24.86458f, 17.76042f, 22.f, 23.25f, 20.3437f, 14.5312f, 18.f, 42.f, 36.75f, 26.25f, 32.0625f, 51.3333f, 44.9167f, 32.08333f, 39.1875f, 42.f, 36.75f, 26.25f, 32.0625f, 4.375f, 4.375f, 4.375f, 4.375f, 7.70833f, 7.70833f, 7.70833f, 7.70833f, 9.375f, 9.375f, 9.375f, 9.375f, 21.875f, 21.875f, 21.875f, 21.875f, 26.9792f, 26.9792f, 26.9792f, 26.9792f, 32.8125f, 32.8125f, 32.8125f, 32.8125f, 40.1042f, 40.1042f, 40.1042f, 40.1042f, 46.25f, 46.25f, 46.25f, 46.25f, 56.25f, 56.25f, 56.25f, 56.25f});

// As per ort issue #3428, the above output values are INCORRECT.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// As per ort issue #3428, the above output values are INCORRECT.
// As per ORT issue https://github.com/microsoft/onnxruntime/issues/6921, the above output values are INCORRECT.

Copy link
Contributor

@fdwr fdwr May 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment was resolved, but it still points to the old incorrect issue (3428 is a random CNTK to ONNX converter bug). Elsewhere too.

fdwr
fdwr previously approved these changes May 5, 2023
fdwr
fdwr previously approved these changes May 5, 2023
@@ -30,7 +29,241 @@ TEST(RoiAlignTest, AvgModePositive) {
test.AddInput<float>("rois", {5, 4}, {7., 5., 7., 5., -15., -15., -15., -15., -10., 21., -10., 21., 13., 8., 13., 8., -14., 19., -14., 19.});
test.AddInput<int64_t>("batch_indices", {5}, {0, 0, 0, 0, 0});
test.AddOutput<float>("Y", {5, 3, 3, 4}, {2.95833f, 3.20833f, 3.45833f, 3.70833f, 4.625f, 4.875f, 5.125f, 5.375f, 6.29167f, 6.54167f, 6.79167f, 7.04167f, 27.9583f, 28.2083f, 28.4583f, 28.7083f, 29.625f, 29.875f, 30.125f, 30.375f, 31.2917f, 31.5417f, 31.7917f, 32.0417f, 52.9583f, 53.2083f, 53.4583f, 53.7083f, 54.625f, 54.875f, 55.125f, 55.375f, 56.2917f, 56.5417f, 56.7917f, 57.0417f, 0.f, 0.f, 0.f, 0.f, 0.f, 0.f, 0.f, 0.f, 0.f, 0.f, 0.f, 0.f, 25.f, 25.f, 25.f, 25.f, 25.f, 25.f, 25.f, 25.f, 25.f, 25.f, 25.f, 25.f, 50.f, 50.f, 50.f, 50.f, 50.f, 50.f, 50.f, 50.f, 50.f, 50.f, 50.f, 50.f, 7.39583f, 7.39583f, 7.42708f, 7.64583f, 9.0625f, 9.0625f, 9.09375f, 9.3125f, 10.7292f, 10.7292f, 10.7604f, 10.9792f, 32.3958f, 32.3958f, 32.4271f, 32.6458f, 34.0625f, 34.0625f, 34.0938f, 34.3125f, 35.7292f, 35.7292f, 35.7604f, 35.9792f, 57.3958f, 57.3958f, 57.4271f, 57.6458f, 59.0625f, 59.0625f, 59.0938f, 59.3125f, 60.7292f, 60.7292f, 60.7604f, 60.9792f, 4.27083f, 4.52083f, 4.77083f, 5.02083f, 5.9375f, 6.1875f, 6.4375f, 6.6875f, 7.60417f, 7.85417f, 8.10417f, 8.35417f, 29.2708f, 29.5208f, 29.7708f, 30.0208f, 30.9375f, 31.1875f, 31.4375f, 31.6875f, 32.6042f, 32.8542f, 33.1042f, 33.3542f, 54.2708f, 54.5208f, 54.7708f, 55.0208f, 55.9375f, 56.1875f, 56.4375f, 56.6875f, 57.6042f, 57.8542f, 58.1042f, 58.3542f, 6.77083f, 6.77083f, 6.77083f, 6.80208f, 8.4375f, 8.4375f, 8.4375f, 8.46875f, 10.1042f, 10.1042f, 10.1042f, 10.1354f, 31.7708f, 31.7708f, 31.7708f, 31.8021f, 33.4375f, 33.4375f, 33.4375f, 33.4688f, 35.1042f, 35.1042f, 35.1042f, 35.1354f, 56.7708f, 56.7708f, 56.7708f, 56.8021f, 58.4375f, 58.4375f, 58.4375f, 58.4688f, 60.1042f, 60.1042f, 60.1042f, 60.1354f});
// As per ORT issue https://github.com/microsoft/onnxruntime/issues/6921, the above output values are INCORRECT.
Copy link
Contributor

@fdwr fdwr May 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would feel cleaner to just replace the existing output values later rather than leave this comment lingering here, or to least add a clarifying comment for readers like...

// DML has the correct outputs, which are defined below.
// These will replace the above outputs once https://github.com/microsoft/onnxruntime/issues/6921 is fixed.

You can also just grab the correct values from https://github.com/microsoft/onnxruntime/pull/7354/files later. Note the NOLINT keyword used in that PR will also yield much more readable formatting than the clangformat bulldozer is aware of.

   "Y",
    {5, 3, 3, 4},
    {
       2.0000,  2.0000,  2.0000,  2.0000,    2.0000,  2.0000,  2.0000,  2.0000,    2.0000,  2.0000,  2.0000,  2.0000,
      27.0000, 27.0000, 27.0000, 27.0000,   27.0000, 27.0000, 27.0000, 27.0000,   27.0000, 27.0000, 27.0000, 27.0000,
      52.0000, 52.0000, 52.0000, 52.0000,   52.0000, 52.0000, 52.0000, 52.0000,   52.0000, 52.0000, 52.0000, 52.0000,

       0.0000,  0.0000,  0.0000,  0.0000,    0.0000,  0.0000,  0.0000,  0.0000,    0.0000,  0.0000,  0.0000,  0.0000,
      25.0000, 25.0000, 25.0000, 25.0000,   25.0000, 25.0000, 25.0000, 25.0000,   25.0000, 25.0000, 25.0000, 25.0000,
      50.0000, 50.0000, 50.0000, 50.0000,   50.0000, 50.0000, 50.0000, 50.0000,   50.0000, 50.0000, 50.0000, 50.0000,

       6.5625,  6.5625,  6.5625,  6.5625,    6.5625,  6.5625,  6.5625,  6.5625,    6.5625,  6.5625,  6.5625,  6.5625,
      31.5625, 31.5625, 31.5625, 31.5625,   31.5625, 31.5625, 31.5625, 31.5625,   31.5625, 31.5625, 31.5625, 31.5625,
      56.5625, 56.5625, 56.5625, 56.5625,   56.5625, 56.5625, 56.5625, 56.5625,   56.5625, 56.5625, 56.5625, 56.5625,

       3.3125,  3.3125,  3.3125,  3.3125,    3.3125,  3.3125,  3.3125,  3.3125,    3.3125,  3.3125,  3.3125,  3.3125,
      28.3125, 28.3125, 28.3125, 28.3125,   28.3125, 28.3125, 28.3125, 28.3125,   28.3125, 28.3125, 28.3125, 28.3125,
      53.3125, 53.3125, 53.3125, 53.3125,   53.3125, 53.3125, 53.3125, 53.3125,   53.3125, 53.3125, 53.3125, 53.3125,

       5.9375,  5.9375,  5.9375,  5.9375,    5.9375,  5.9375,  5.9375,  5.9375,    5.9375,  5.9375,  5.9375,  5.9375,
      30.9375, 30.9375, 30.9375, 30.9375,   30.9375, 30.9375, 30.9375, 30.9375,   30.9375, 30.9375, 30.9375, 30.9375,
      55.9375, 55.9375, 55.9375, 55.9375,   55.9375, 55.9375, 55.9375, 55.9375,   55.9375, 55.9375, 55.9375, 55.9375,
    }
  ); // NOLINT

smk2007
smk2007 previously approved these changes May 9, 2023
@linnealovespie
Copy link
Contributor Author

dml roialign-16

@linnealovespie linnealovespie reopened this May 9, 2023
@linnealovespie linnealovespie merged commit 95a4607 into main May 10, 2023
@linnealovespie linnealovespie deleted the user/linneamay/roi-align-16 branch May 10, 2023 04:56
linnealovespie added a commit that referenced this pull request May 10, 2023
### 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]>
snnn pushed a commit that referenced this pull request May 11, 2023
### 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]>
@fdwr fdwr changed the title User/linneamay/roi align 16 DML EP User/linneamay/roi align 16 May 11, 2023
@fdwr fdwr changed the title DML EP User/linneamay/roi align 16 DML EP RoiAlign-16 May 11, 2023
fs-eire pushed a commit that referenced this pull request May 12, 2023
### 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]>
@snnn snnn added triage:approved Approved for cherrypicks for release and removed triage:approved Approved for cherrypicks for release release:1.15 labels May 18, 2023
@snnn
Copy link
Member

snnn commented May 19, 2023

already in

preetha-intel pushed a commit to intel/onnxruntime that referenced this pull request Jun 7, 2023
### 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]>
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.

4 participants