-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Conversation
eacda4d
to
7849b3b
Compare
@mxnet-label-bot add [pr-awaiting-review, Operator] |
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.
Retrigger the CI as the CI issue of test_stn has been fixed 2 days back by #14063
git commit --allow-empty -m "Trigger notification"
@@ -106,7 +109,7 @@ inline bool BoxNMSShape(const nnvm::NodeAttrs& attrs, | |||
<< ishape << " provided"; | |||
int width_elem = ishape[indim - 1]; | |||
int expected = 5; | |||
if (param.id_index > 0) { | |||
if (param.id_index >= 0) { |
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.
curious to know why this change?
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.
I think the valid id_index should be >= 0 instead of just > 0. Surely, this line is not relevant to this PR.
@mxnet-label-bot update [pr-awaiting-response, Operator] |
943ea0d
to
f36efee
Compare
@apeforest @samskalicky Coul d you please help review this PR? |
@arcadiaphy please add tests for these changes. |
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.
Excellent. Thanks for your contribution!
LGTM: )
@wkcn CI seems OK now, ready for merge. |
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.
would marking scores of all boxes with background_id to 0 or -1 simpler and don't require any operator change?
@zhreshold Currently there are no background_id logic in box_nms operator on prefiltering or nms step, so a lot of time is wasted in processing background boxes. To keep operator unchanged, background boxes need to be removed in NDArray input. |
@arcadiaphy Yes, a pre-filtering can be much easier, but there's no guarantee that box_nms will always have clean input. So I am actually convinced this is good to go. BTW, do you have the new validation time with this new PR? |
@zhreshold Sometimes the validation time of SSD_512 on voc dataset is more than half an hour before, much longer than training time. I have written a simple script to show benchmark:
The output on TITAN X (Pascal):
|
@arcadiaphy Please also take a look at two improvements contributed by @ptrendx |
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.
waiting for justification of the added API
@zhreshold My PR is complementary to the great improvements of @ptrendx on cuda kernels. After combining all three PRs, |
@arcadiaphy Please resolve the conflict and it's good to go. 😺 |
@zhreshold OK to go now. |
thanks @arcadiaphy , this is merged |
* add backgroud class in box_nms * add unittest * trigger CI
* add backgroud class in box_nms * add unittest * trigger CI
* add backgroud class in box_nms * add unittest * trigger CI
Description
This PR is mentioned in #14057
What I have done in box_nms operator:
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
Comments