-
Notifications
You must be signed in to change notification settings - Fork 3.8k
[relay][frontend] Enable ssd test by attaching schedules to multibox and ssd ops #2322
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
Conversation
24223c0 to
c5480d5
Compare
include/tvm/relay/attrs/vision.h
Outdated
|
|
||
| TVM_DECLARE_ATTRS(NMSAttrs, "relay.attrs.NMSAttrs") { | ||
| TVM_ATTR_FIELD(overlap_threshold).set_default(0.5) | ||
| TVM_ATTR_FIELD(nms_threshold).set_default(0.5) |
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.
We should keep this argument name as overlap_threshold, since we don't need nms prefix. It can generate some inconsistency with current topi nms operator, but topi nms argument names will be modified in SSD refactor PR.
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.
Thanks for pointing this out. I intentionally changed them to keep consistent to top. I will change them back.
| return [ | ||
| topi.vision.nms(inputs[0], inputs[1], nms_threshold, force_suppress, | ||
| nms_topk) | ||
| ] |
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.
This is not needed?
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.
This is needed because fcompute needs to receive an Array of Tensor when returning to c++. We can probably refactor the packfunc conversion a little bit to accept both Array and Tensor.
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.
NNVM automatically wraps result in Array:
https://github.com/dmlc/tvm/blob/e5d92e1b96c1fbc175be741f522c030f2d91a613/nnvm/src/compiler/packed_func_ext.cc#L99-L103
But Relay does not:
https://github.com/dmlc/tvm/blob/e5d92e1b96c1fbc175be741f522c030f2d91a613/src/relay/ir/op.cc#L129-L135
Relay doc asks for schedule function that returns list. So this is indeed complying with existing Relay interface.
|
Thanks @zhiics @kevinthesun @wweic This is now merged. |
…and ssd ops (apache#2322) * add ssd ops to mxnet.py * add ssd ops to mxnet.py * add result check for multibox and nms unit tests * add result check for multibox and nms unit tests * address @kevinthesun's comments * Disable cuda test for nms for now.
…and ssd ops (apache#2322) * add ssd ops to mxnet.py * add ssd ops to mxnet.py * add result check for multibox and nms unit tests * add result check for multibox and nms unit tests * address @kevinthesun's comments * Disable cuda test for nms for now.
…and ssd ops (apache#2322) * add ssd ops to mxnet.py * add ssd ops to mxnet.py * add result check for multibox and nms unit tests * add result check for multibox and nms unit tests * address @kevinthesun's comments * Disable cuda test for nms for now.
This PR enables ssd test by attaching the
fcomputeandfscheduleimplementations in TOPI tomultibox_priorandnmsoperators.It also implements the customized
multibox_priorandmultibox_detectionops in mxnet.py to enable support of conversion from MxNet.tutorial/nnvm/deploy_ssd.pyis used to verify the above changes.@tqchen @kevinthesun @yzhliu