-
Notifications
You must be signed in to change notification settings - Fork 87
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
Rectify flipped coordinate_transformation_mode logic in ROIAlign #2159
Conversation
music-dino
commented
Sep 7, 2023
- Resolves ROIAlign inaccuracies migraphx-benchmark/AMDMIGraphX#77
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test_roialign_aligned_false_cpu
test_roialign_aligned_true_cpu
test_roialign_mode_max_cpu
Are these test from onnx_backend tests ?
Codecov Report
@@ Coverage Diff @@
## develop #2159 +/- ##
========================================
Coverage 91.49% 91.49%
========================================
Files 427 427
Lines 15953 15953
========================================
Hits 14596 14596
Misses 1357 1357
|
Yes. They were added in an ONNX version that came after 1.10.2, so they only fail when the ONNX version is bumped. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it was a typo where the tests were also changed to agree with the typo
This PR is breaking ONNX tests [2023-09-15T19:21:13.577Z] 333/333 Test #332: test_py_3.8_backend .......................................................***Failed 1046.20 sec
|
Steps to recreate... [2023-09-15T18:48:08.649Z] ulimit -c unlimited [2023-09-15T18:48:08.649Z] make -j$(nproc) check VERBOSE=1 |
That's an oversight on my part. The test in question is not part of ONNX 1.14, the version for which I had run the backend tests, neglecting to run them for 1.10.2. With that in mind perhaps it would be best if #2121 were given priority for merging. |
Looks like the default behavior of RoiAlign changed between opset versions 10 and 16 onnx/onnx#3625. |
@causten Seems like some checks need to be triggered again. |
Yeah I'm not seeing an option for the PR to get run. Ttrying to figure it out |
@music-dino, there has been a recent change to speed up CI builds which unfortunately uses a github secret. Since your PR came from a fork it will fail some CI tests. I created a local version of your PR #2214 |
#2214) * Rectify flipped coordinate_transformation_mode logic in ROIAlign * Handle both opset 10 and 16 versions * Fix version check and clang tidy warning Co-authored-by: Dino Musić <[email protected]>