Skip to content
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

Merged
merged 2 commits into from
Jul 27, 2020
Merged

[Relay][OP] Support NMSv4 #6085

merged 2 commits into from
Jul 27, 2020

Conversation

csullivan
Copy link
Contributor

Add support for pad_to_max_output_size field in NMSv4 from Tensorflow which results in two outputs:

  1. Indices tensor (vector of length max_num_indices.
  2. Scalar tensor indicating the number of valid entries in the indices tensor.


# Generate data with shape (1, num_anchors, 5)
scores = AttrCvt(op_name="expand_dims",
ignores=['T_threshold'],
ignores=['T_threshold', pad_output],
Copy link
Contributor

@kevinthesun kevinthesun Jul 18, 2020

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. LGTM.

Copy link
Contributor

@kevinthesun kevinthesun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kevinthesun kevinthesun merged commit abc52aa into apache:master Jul 27, 2020
@kevinthesun
Copy link
Contributor

Thanks @csullivan

trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Aug 26, 2020
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Aug 26, 2020
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Aug 26, 2020
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Sep 2, 2020
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Sep 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: need review status: need update need update based on feedbacks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants