-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Relay][OP] Support NMSv4 #6085
Conversation
|
||
# Generate data with shape (1, num_anchors, 5) | ||
scores = AttrCvt(op_name="expand_dims", | ||
ignores=['T_threshold'], | ||
ignores=['T_threshold', pad_output], |
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 want to do padding if number of output boxes is smaller than max_output_size
?
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.
NMSv4 semantics allow for the user to define whether or not to pad the output using pad_to_max_output_size=True/False
, see the example I provided in the test. Some pre-trained TF models use NMSv4 with pad_to_max_output_size=True, in which case, yes, they expect padding of the indices if the number of boxes is less than max_output_size
, as well as an additional scalar output specifying the number of valid boxes.
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 need to handle True
case instead of just ignoring it?
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.
It's not ignored, this PR adds explicit support to __nms()
for the pad_to_max_output_size=True
case, see lines 671/2.
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.
Got it. LGTM.
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.
LGTM
Thanks @csullivan |
Add support for
pad_to_max_output_size
field in NMSv4 from Tensorflow which results in two outputs: