[Relay][OP]NMS#1929
Conversation
|
please rebase against master as the recent API stablization in #1934 |
|
|
||
| /*! \brief Attributes used in non_maximum_suppression operators */ | ||
| struct NMSAttrs : public tvm::AttrsNode<NMSAttrs>{ | ||
| double nms_threshold; |
There was a problem hiding this comment.
do we need nms prefix here? as it is already an nms attribute
There was a problem hiding this comment.
suggest using overlap_threshold, for separation with score_threshold, etc.
|
@kevinthesun please request reviews |
|
@nishi-t @zhreshold can you please review this code? |
| nms_threshold=0.5, | ||
| force_suppress=False, | ||
| nms_topk=-1): | ||
| """Generate prior(anchor) boxes from data, sizes and ratios. |
|
|
||
| /*! \brief Attributes used in non_maximum_suppression operators */ | ||
| struct NMSAttrs : public tvm::AttrsNode<NMSAttrs>{ | ||
| double nms_threshold; |
There was a problem hiding this comment.
suggest using overlap_threshold, for separation with score_threshold, etc.
|
|
||
| def nms(data, | ||
| valid_count, | ||
| nms_threshold=0.5, |
There was a problem hiding this comment.
same, suggesting overlap_threshold
| The last dimension should be in format of | ||
| [class_id, score, box_left, box_top, box_right, box_bottom]. | ||
|
|
||
| valid_count : relay.Expr |
There was a problem hiding this comment.
why is valid_count used here?
There was a problem hiding this comment.
The methods multibox_detection and box_nms generating valid_cout are slightly different. This NMS is a general interface to support both cases. I put valid_count as an argument so that we don't need to add extra flag to differentiate multibox_detection and box_nms use cases.
| [class_id, score, box_left, box_top, box_right, box_bottom]. | ||
|
|
||
| valid_count : relay.Expr | ||
| Number of valid anchor boxes. |
There was a problem hiding this comment.
It might be more understandable if information which valid_count is 1-D tensor is specified as with the parameter description in topi's nms.
1-D tensor for valid number of boxes.
kevinthesun
force-pushed
the
RelayNMSOperator
branch
from
3eb684d to
777352e
Compare
| valid_count : relay.Expr | ||
| 1-D tensor for valid number of boxes. | ||
|
|
||
| nms_threshold : float, optional |
There was a problem hiding this comment.
@kevinthesun please update the argument list to reflect the parameters here
|
Thanks, @kevinthesun @nishi-t @zhreshold for reviewing and contributing. There has been some flux in changes of parameter naming, given that nms is not a standard operator. I would like everyone to have a discussion again about the parameter naming Specifically, there is a bit inconsistency in current naming
Let us also refer to existing implementations and try to be consistent as possible |
|
@kevinthesun can you follow up on this? |
kevinthesun
force-pushed
the
RelayNMSOperator
branch
from
a9e1888 to
8dbdaec
Compare
|
@tqchen I rename "mms_topk" to "mms". Now no argument has mms as prefix. |
|
Thanks @kevinthesun this is now merged |
tqchen
added
status: accepted
and removed
status: need review
status: need update
4 participants
Relay non-maximum suppression operator.