-
Notifications
You must be signed in to change notification settings - Fork 88
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
Add support for Shrink ONNX operator #2240
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2240 +/- ##
===========================================
+ Coverage 91.35% 91.36% +0.01%
===========================================
Files 438 439 +1
Lines 16465 16493 +28
===========================================
+ Hits 15041 15069 +28
Misses 1424 1424
|
A new migraphx operator should not be created for this. This should use existing operators that should be inserted during parse_onnx. |
Thanks @pfultz2 I've changed the PR according to that. |
ce152e5
to
c5b573f
Compare
a30cc59
to
fdc4caa
Compare
src/onnx/parse_shrink.cpp
Outdated
auto filtered = | ||
info.add_instruction(migraphx::make_op("where"), condition_1, x_plus_bias, mb_zero); | ||
return info.add_instruction(migraphx::make_op("where"), condition_2, x_min_bias, filtered); |
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.
ONNX spec is unclear to me if they want:
if(x < -lambd){ y = x + bias )
if(x > lambd){ y = x -bias }
else { y = 0}
or
if(x < -lambd){ y = x + bias )
else if(x > lambd){ y = x -bias }
else { y = 0}
It looks like this implements the else if
version
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.
Good question for example x=0
and lamd=-5
satisfies both conditions . I will check the reference implementation and come back with an answer.
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.
Seems like onnxruntime uses the if
,else if
,else
solution (they are using return in each branch): https://github.com/microsoft/onnxruntime/blob/25bbd8d4ebed60c28f702a9fe6b0e6a12c19de9c/onnxruntime/core/providers/cpu/nn/shrink.cc#L37-L47
fdc4caa
to
3f3ac05
Compare
@umangyadav I've added the requested test cases to check the conversion handling. |
There is a CI failure |
[ RUN ] test_shrinkmigraphx::shape::uint64_type /usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/stl_algobase.h:342:20: runtime error: -3 is outside the range of representable values of type 'unsigned long'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/stl_algobase.h:342:20 in CMake Error at gdb/test_test_verify_general/run.cmake:16 (message): Test failed |
dc7accf
to
5377305
Compare
@causten the CI fail should be fixed now |
Note: depends on #2179, until it's merged marked as Draft
This PR adds support for Shrink ONNX operator.
Resolves migraphx-benchmark#118