You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/04/05 06:00:44 UTC
[GitHub] [tvm] masahi opened a new pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX / TF Combined NMS
masahi opened a new pull request #7796:
URL: https://github.com/apache/tvm/pull/7796
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi edited a comment on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX NMS / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
masahi edited a comment on pull request #7796:
URL: https://github.com/apache/tvm/pull/7796#issuecomment-816367364
Thanks @trevor-m
1. Yes, we should be able to support such variant easily. The only place we touch box coords is here (`i` is the batch or row index, j and k are the index of sorted scores) https://github.com/apache/tvm/blob/6d314deb49898967361f62c194c9ea3685961dfd/python/tvm/topi/vision/nms_util.py#L132-L141 we can do arbitrary indirect indexing into boxes. We might need to update `calculate_overlap` function but that would be easy as well.
2. Yes, the new NMS will return indices, and it would be the job of frontend importer to do the gather. We can discuss what format of output would be most convenient for TF, and add `ret_type` attribute to specify which of ONNX or TF "mode" we return indices in.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] trevor-m edited a comment on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX NMS / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
trevor-m edited a comment on pull request #7796:
URL: https://github.com/apache/tvm/pull/7796#issuecomment-816306316
Really nice! Just did first pass through the code and overall it looks good.
A couple of thoughts/questions:
1. TF Combined NMS allows for both a) using same boxes for all classes and b) using different boxes for each class.
For a) the shape of boxes input is `(batch_size, 1, num_boxes, 4)`.
For b) the shape of boxes input is `(batch_size, num_classes, num_anchors, 4)
`.
Currently, your implementation only supports a) and has a box input shape of (batch_size, num_anchors, 4).
I think all of the uses of combinedNMS I have seen in TF have only used a), but perhaps in the future we may want to also add support for b).
2. This is regarding the output format. One common thing after getting the selected indices is using those to gather the coordinates and scores of the selected boxes.
Since the selected indices are in the format `[batch_id, class_id, index]` we can actually use them directly to index into the scores tensors which has shape `(batch, num_classes, num_boxes)` to get the selected scores.
But for the selected boxes, we need to slice out the batch_id and index and concat to get them into the format `[batch_id, index]` before we can index into `boxes` tensor to get selected boxes.
Is my understanding here correct? I'm wondering if we can improve this somehow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen edited a comment on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #7796:
URL: https://github.com/apache/tvm/pull/7796#issuecomment-813511944
awesome. it would be great to also discuss the naming of the API(by checking with the naming in the existing libraries e.g. TF, Pytorch). e.g. shall we call it `combined_non_max_suppression` as per TF's naming convention?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX NMS / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7796:
URL: https://github.com/apache/tvm/pull/7796#issuecomment-818997942
Yes, the way it always works is we first implement topi op with tests, and then relay op. So topi tests always come first. Two tests are indeed redundant and ideally we should have difference test cases. But I'd say it is also weird to delete topi tests on purpose after we have Relay tests. It doesn't hurt to leave topi tests anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX NMS / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7796:
URL: https://github.com/apache/tvm/pull/7796#issuecomment-817021381
@mbrookhart @jwfromm @trevor-m @Laurawly ready for review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7796:
URL: https://github.com/apache/tvm/pull/7796#issuecomment-813636766
> I'm seeing some removed attributes from the normal NMS op, I didn't add those as part of the ONNX NMS PR, I imagine they're used by other frontends?
@mbrookhart These attributes are now runtime arguments and they are not used anymore, see https://github.com/apache/tvm/blob/813136401a11a49d6c15e6013c34dd822a5c4ff6/src/relay/op/vision/nms.cc#L106-L113 To avoid confusion I removed them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] trevor-m edited a comment on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX NMS / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
trevor-m edited a comment on pull request #7796:
URL: https://github.com/apache/tvm/pull/7796#issuecomment-816306316
Really nice! Just did first pass through the code and overall it looks good.
A couple of thoughts/questions:
1. TF Combined NMS allows for both a) using same boxes for all classes and b) using different boxes for each class.
For a) the shape of boxes input is `(batch_size, 1, num_boxes, 4)`.
For b) the shape of boxes input is `(batch_size, num_classes, num_anchors, 4)
`.
Currently, your implementation only supports a) and has a box input shape of (batch_size, num_anchors, 4).
I think all of the uses of combinedNMS I have seen in TF have only used a), but perhaps in the future we may want to also add support for b).
2. This is regarding the output format. One common thing after getting the selected indices is using those to gather the coordinates and scores of the selected boxes.
Since the selected indices are in the format `[batch_id, class_id, index]` we can actually use them directly to index into the scores tensors which has shape `(batch, num_classes, num_boxes)` to get the selected scores.
But for the selected boxes, we need to slice out the batch_id and index and concat to get them into the format `[batch_id, index]` before we can index into `boxes` tensor to get selected boxes.
Is my understanding here correct?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #7796:
URL: https://github.com/apache/tvm/pull/7796#issuecomment-813511944
awesome. it would be great to also discuss the naming of the API. e.g. shall we call it `combined_non_max_suppression` as per TF's naming convention?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX NMS / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7796:
URL: https://github.com/apache/tvm/pull/7796#issuecomment-819769999
Thanks @mbrookhart @trevor-m @tqchen @electriclilies
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi edited a comment on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
masahi edited a comment on pull request #7796:
URL: https://github.com/apache/tvm/pull/7796#issuecomment-814840398
ok I see roughly three categories of NMS used in frameworks:
1. MXNet and TVM https://mxnet.apache.org/versions/1.7.0/api/python/docs/api/ndarray/contrib/index.html#mxnet.ndarray.contrib.box_nms This is what MXNet calls `box_nms`, this API is highly non-standard but unfortunately this is what we inherited from for topi/relay. It supports multiclass NMS but a single box is associated with only one class, unlike the third category below.
2. PT `torchvision.ops.nms` and TF `non_max_suppression`, this is the standard, **single** class NMS.
https://pytorch.org/vision/stable/ops.html#torchvision.ops.nms
https://www.tensorflow.org/api_docs/python/tf/image/non_max_suppression
3. ONNX `NonMaxSuppression`, TF `combined_non_max_suppression`, and TensorRT `batchedNMSPlugin` (the implementation calls it `allClassNMS`) This is the variant of **multi** class NMS where a single box can be selected multiple times per different classes.
https://github.com/onnx/onnx/blob/master/docs/Operators.md#NonMaxSuppression
https://www.tensorflow.org/api_docs/python/tf/image/combined_non_max_suppression
https://github.com/NVIDIA/TensorRT/tree/master/plugin/batchedNMSPlugin, https://github.com/NVIDIA/TensorRT/blob/master/plugin/common/kernels/allClassNMS.cu
So the bottom line is, I think it is reasonable to say that NMS, without adjectives, should refer to the single class variant (category 2 above), but there isn't a consensus on what to call the third category one which this PR is about.
I think `all_class_non_maximum_surpression` or `per_class_non_maximum_surpression` are the most descriptive of what it does. Either one is fine for me and I'm open to other suggestions @mbrookhart @jwfromm
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] trevor-m edited a comment on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX NMS / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
trevor-m edited a comment on pull request #7796:
URL: https://github.com/apache/tvm/pull/7796#issuecomment-816306316
Really nice! Just did first pass through the code and overall it looks good.
A couple of thoughts/questions:
1. TF Combined NMS allows for both a) using same boxes for all classes and b) using different boxes for each class.
For a) the shape of boxes input is `(batch_size, 1, num_boxes, 4)`.
For b) the shape of boxes input is `(batch_size, num_classes, num_anchors, 4)
`.
Currently, your implementation only supports a) and has a box input shape of (batch_size, num_anchors, 4).
I think all of the uses of combinedNMS I have seen in TF have only used a), but perhaps in the future we may want to also add support for b).
2. This is regarding the output format. One common thing after getting the selected indices is using those to gather the coordinates and scores of the selected boxes.
Since the selected indices are in the format `[batch_id, class_id, index]` we can actually use them directly to index into the scores tensors which has shape `(batch, num_classes, num_boxes)` to get the selected scores.
But for the selected boxes, we need to slice out the batch_id and index and concat to get them into the format `[batch_id, index]` before we can index into `boxes` tensor to get selected boxes.
Is my understanding here correct?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7796:
URL: https://github.com/apache/tvm/pull/7796#issuecomment-813639924
> awesome. it would be great to also discuss the naming of the API(by checking with the naming in the existing libraries e.g. TF, Pytorch). e.g. shall we call it `combined_non_max_suppression` as per TF's naming convention?
`combined_non_max_suppression` was also the name I started with, but I didn't particularly like it because it doesn't tell what exactly is "combined".
Moreover, in TF [the other NMS op](https://www.tensorflow.org/api_docs/python/tf/image/non_max_suppression) is the standard, single class NMS, so for them combined or not is really single vs multiclass distinction. In contrast, our existing NMS also does multiclass NMS where a single box is associated with a single class. This variant of multiclass NMS can support PyTorch and MXNet. So I don't think `combined_nms` is a good choice for us.
`all_class_non_maximum_surpression` is also what TensorRT calls it https://github.com/NVIDIA/TensorRT/blob/master/plugin/common/kernels/allClassNMS.cu I don't have a better alternative than this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mbrookhart commented on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX NMS / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on pull request #7796:
URL: https://github.com/apache/tvm/pull/7796#issuecomment-815292874
I think that sounds good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] trevor-m commented on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX NMS / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
trevor-m commented on pull request #7796:
URL: https://github.com/apache/tvm/pull/7796#issuecomment-819762033
Thanks @masahi
Once this is merged I will open a PR for the TF frontend
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX NMS / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7796:
URL: https://github.com/apache/tvm/pull/7796#issuecomment-818985020
Yes it is redundant, but it's simply too tedious to come up with good test cases for this op. Do you have any suggestion on how to go about this? Ideally I want to test on real workload, a small artificial test case like that doesn't really exercise all the tricky aspects in our NMS kernel.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi edited a comment on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
masahi edited a comment on pull request #7796:
URL: https://github.com/apache/tvm/pull/7796#issuecomment-813639924
> awesome. it would be great to also discuss the naming of the API(by checking with the naming in the existing libraries e.g. TF, Pytorch). e.g. shall we call it `combined_non_max_suppression` as per TF's naming convention?
`combined_non_max_suppression` was also the name I started with, but I didn't particularly like it because it doesn't tell what exactly is "combined".
Moreover, in TF [the other NMS op](https://www.tensorflow.org/api_docs/python/tf/image/non_max_suppression) is the standard, single class NMS, so for them combined or not is really single vs multiclass distinction. In contrast, our existing NMS also does multiclass NMS where a single box is associated with a single class. This variant of multiclass NMS has a different notion of "multiclass" and can support PyTorch and MXNet. So I don't think `combined_nms` is a good choice for us.
`all_class_non_maximum_surpression` is also what TensorRT calls it https://github.com/NVIDIA/TensorRT/blob/master/plugin/common/kernels/allClassNMS.cu I don't have a better alternative than this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi edited a comment on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
masahi edited a comment on pull request #7796:
URL: https://github.com/apache/tvm/pull/7796#issuecomment-814840398
ok I see roughly three categories of NMS used in frameworks:
1. MXNet and TVM https://mxnet.apache.org/versions/1.7.0/api/python/docs/api/ndarray/contrib/index.html#mxnet.ndarray.contrib.box_nms This is what MXNet calls `box_nms`, this API is highly non-standard but unfortunately this is what we inherited from for topi/relay. It supports multiclass NMS but a single box is associated with only one class, unlike the third category below.
2. PT `torchvision.ops.nms` and TF `non_max_suppression`, this is the standard, **single** class NMS.
https://pytorch.org/vision/stable/ops.html#torchvision.ops.nms
https://www.tensorflow.org/api_docs/python/tf/image/non_max_suppression
3. ONNX `NonMaxSuppression`, TF `combined_non_max_suppression`, and TensorRT `batchedNMSPlugin` (the implementation calls it `allClassNMS`) This is the variant of **multi** class NMS where a single box can be selected multiple times per different classes.
https://github.com/onnx/onnx/blob/master/docs/Operators.md#NonMaxSuppression
https://www.tensorflow.org/api_docs/python/tf/image/combined_non_max_suppression
https://github.com/NVIDIA/TensorRT/tree/master/plugin/batchedNMSPlugin, https://github.com/NVIDIA/TensorRT/blob/master/plugin/common/kernels/allClassNMS.cu
So the bottom line is, I think it is reasonable to say that NMS, without adjectives, should refer to the single class variant (category 2 above), but there isn't a consensus on what to call the third category one which this PR is about.
I think `all_class_non_maximum_surpression` or `per_class_non_maximum_surpression` are the most descriptive.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi edited a comment on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX NMS / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
masahi edited a comment on pull request #7796:
URL: https://github.com/apache/tvm/pull/7796#issuecomment-816367364
Thanks @trevor-m
1. Yes, we should be able to support such variant easily. The only place we touch box coords is here (`i` is the batch or row index, j and k are the index of sorted scores) https://github.com/apache/tvm/blob/6d314deb49898967361f62c194c9ea3685961dfd/python/tvm/topi/vision/nms_util.py#L132-L141 we can do arbitrary indirect indexing into boxes
2. Yes, the new NMS will return indices, and it would be the job of frontend importer to do the gather. We can discuss what format of output would be most convenient for TF, and add `ret_type` attribute to specify which of ONNX or TF "mode" we return indices in.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi edited a comment on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
masahi edited a comment on pull request #7796:
URL: https://github.com/apache/tvm/pull/7796#issuecomment-814840398
ok I see roughly three categories of NMS used in frameworks:
1. MXNet and TVM https://mxnet.apache.org/versions/1.7.0/api/python/docs/api/ndarray/contrib/index.html#mxnet.ndarray.contrib.box_nms This is what MXNet calls `box_nms`, this API is highly non-standard but unfortunately this is what we inherited from for topi/relay. It supports multiclass NMS but a single box is associated with only one class, unlike the third category below.
2. PT `torchvision.ops.nms` and TF `non_max_suppression`, this is a standard, **single** class NMS.
https://pytorch.org/vision/stable/ops.html#torchvision.ops.nms
https://www.tensorflow.org/api_docs/python/tf/image/non_max_suppression
3. ONNX `NonMaxSuppression`, TF `combined_non_max_suppression`, and TensorRT `batchedNMSPlugin` (the implementation calls it `allClassNMS`) This is the variant of **multi** class NMS where a single box can be selected multiple times per different classes.
https://github.com/onnx/onnx/blob/master/docs/Operators.md#NonMaxSuppression
https://www.tensorflow.org/api_docs/python/tf/image/combined_non_max_suppression
https://github.com/NVIDIA/TensorRT/tree/master/plugin/batchedNMSPlugin, https://github.com/NVIDIA/TensorRT/blob/master/plugin/common/kernels/allClassNMS.cu
So the bottom line is, I think it is reasonable to say that NMS, without adjectives, should refer to the single class variant (category 2 above), but there isn't a consensus on what to call the third category one which this PR is about.
I think `all_class_non_maximum_surpression` or `per_class_non_maximum_surpression` are the most descriptive.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi edited a comment on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX NMS / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
masahi edited a comment on pull request #7796:
URL: https://github.com/apache/tvm/pull/7796#issuecomment-816367364
Thanks @trevor-m
1. Yes, we should be able to support such variant easily. The only place we touch box coords is here (`i` is the batch or row index, j and k are the index of sorted scores) https://github.com/apache/tvm/blob/6d314deb49898967361f62c194c9ea3685961dfd/python/tvm/topi/vision/nms_util.py#L132-L141 we can do arbitrary indirect indexing into boxes. We might need to update `calculate_overlap` function but that would be easy as well.
2. Yes, the new NMS will return indices, and it would be the job of frontend importer to do the gather. For TF, in particular, the need to return per batch output would be a bit annoying. We can discuss what format of output would be most convenient for TF, and add `ret_type` attribute to specify which of ONNX or TF "mode" we return indices in.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi edited a comment on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
masahi edited a comment on pull request #7796:
URL: https://github.com/apache/tvm/pull/7796#issuecomment-814840398
ok I see roughly three categories of NMS used in frameworks:
1. MXNet and TVM https://mxnet.apache.org/versions/1.7.0/api/python/docs/api/ndarray/contrib/index.html#mxnet.ndarray.contrib.box_nms This is what MXNet calls `box_nms`, this API is highly non-standard but unfortunately this is what we inherited from for topi/relay. It supports multiclass NMS but a single box is associated with only one class, unlike the third category below.
2. PT `torchvision.ops.nms` and TF `non_max_suppression`, this is the standard, **single** class NMS.
https://pytorch.org/vision/stable/ops.html#torchvision.ops.nms
https://www.tensorflow.org/api_docs/python/tf/image/non_max_suppression
3. ONNX `NonMaxSuppression`, TF `combined_non_max_suppression`, and TensorRT `batchedNMSPlugin` (the implementation calls it `allClassNMS`) This is the variant of **multi** class NMS where a single box can be selected multiple times per different classes.
https://github.com/onnx/onnx/blob/master/docs/Operators.md#NonMaxSuppression
https://www.tensorflow.org/api_docs/python/tf/image/combined_non_max_suppression
https://github.com/NVIDIA/TensorRT/tree/master/plugin/batchedNMSPlugin, https://github.com/NVIDIA/TensorRT/blob/master/plugin/common/kernels/allClassNMS.cu
So the bottom line is, I think it is reasonable to say that NMS, without adjectives, should refer to the single class variant (category 2 above), but there isn't a consensus on what to call the third category one which this PR is about.
I think `all_class_non_maximum_surpression` or `per_class_non_maximum_surpression` are the most descriptive of what it does. Either one is fine for me and I'm open for other suggestions @mbrookhart @jwfromm
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX NMS / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7796:
URL: https://github.com/apache/tvm/pull/7796#issuecomment-816367364
Thanks @trevor-m
1. Yes, we should be able to support such variant easily. The only place we touch box coords is here https://github.com/apache/tvm/blob/6d314deb49898967361f62c194c9ea3685961dfd/python/tvm/topi/vision/nms_util.py#L132-L141 we can do arbitrary indirect indexing into boxes
2. Yes, the new NMS will return indices, and it would be the job of frontend importer to do the gather. We can discuss what format of output would be most convenient for TF, and add `ret_type` attribute to specify which of ONNX or TF "mode" we return indices in.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX NMS / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7796:
URL: https://github.com/apache/tvm/pull/7796#issuecomment-815276660
By `per_class` I meant we do NMS for each class separately (unlike our existing NMS), but if this is already confusing then yes, `all_class_non_maximum_supression` might be better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi merged pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX NMS / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
masahi merged pull request #7796:
URL: https://github.com/apache/tvm/pull/7796
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] electriclilies commented on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX NMS / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
electriclilies commented on pull request #7796:
URL: https://github.com/apache/tvm/pull/7796#issuecomment-815266404
I think that the name `per_class_non_maximum_supression` implies that there is one class label per bounding box, but the ONNX version allows a box to be selected by multiple classes, so I prefer `all_class_non_maximum_supression` over `per_class_non_maximum_supression`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #7796:
URL: https://github.com/apache/tvm/pull/7796#issuecomment-814052819
Thanks @masahi , can you do a quick round of search of pytorch, onnx, tensorrt and others and summarize the naming rationale? The main thing is to write down why we pick the name, I am less attached to a particular name.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] trevor-m commented on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX NMS / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
trevor-m commented on pull request #7796:
URL: https://github.com/apache/tvm/pull/7796#issuecomment-816306316
Really nice! Just did first pass through the code and overall it looks good.
A couple of thoughts/questions:
1. TF Combined NMS allows for both a) using same boxes for all classes and b) using different boxes for each class.
For a) the shape of boxes input is `(batch_size, 1, num_boxes, 4)`.
For b) the shape of boxes input is `(batch_size, num_classes, num_anchors, 4)
`.
Currently, your implementation only supports a) and has a box input shape of (batch_size, num_anchors, 4).
I think all of the uses of combinedNMS I have seen in TF have only used a), but perhaps in the future we may want to also add support for b).
2. This is regarding the output format. One common thing after getting the selected indices is using those to gather the coordinates and scores of the selected boxes.
Since the selected indices are in the format `[batch_id, class_id, index]` we can actually use them directly to index into the scores tensors which has shape `(batch, num_classes, num_boxes)` to get the selected scores.
But for the selected boxes, we need to slice out the batch_id and index and concat to get them into the format `[batch_id, index]` before we can index into `boxes` tensor to get selected boxes.
Is my understanding here correct?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mbrookhart commented on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX NMS / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on pull request #7796:
URL: https://github.com/apache/tvm/pull/7796#issuecomment-818989858
Honestly, I'd be okay with just dropping the topi tests, they seem especially redundant after relay. Perhaps there's a TF test or two we could include with the TF importer? I'm planning on dropping the onnx tests once I get the imported node tests working on more backends.
Again, this a nitpick, I don't think it needs to be in this PR, I just think we generally have too much redundancy in testing and not enough coverage
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7796:
URL: https://github.com/apache/tvm/pull/7796#issuecomment-814840398
ok I see roughly three categories of NMS used in frameworks:
1. MXNet and TVM https://mxnet.apache.org/versions/1.7.0/api/python/docs/api/ndarray/contrib/index.html#mxnet.ndarray.contrib.box_nms This is what MXNet calls `box_nms`, this API is highly non-standard but unfortunately this is what we inherited from for topi/relay. It supports multiclass NMS but a single box is associated with only one class, compared to the third category below.
2. PT `torchvision.ops.nms` and TF `non_max_suppression`, this is a standard, **single** class NMS.
https://pytorch.org/vision/stable/ops.html#torchvision.ops.nms
https://www.tensorflow.org/api_docs/python/tf/image/non_max_suppression
3. ONNX `NonMaxSuppression`, TF `combined_non_max_suppression`, and TensorRT `batchedNMSPlugin` (the implementation calls it `allClassNMS`) This is the variant of **multi** class NMS where a single box can be selected multiple times per different classes.
https://github.com/onnx/onnx/blob/master/docs/Operators.md#NonMaxSuppression
https://www.tensorflow.org/api_docs/python/tf/image/combined_non_max_suppression
https://github.com/NVIDIA/TensorRT/tree/master/plugin/batchedNMSPlugin, https://github.com/NVIDIA/TensorRT/blob/master/plugin/common/kernels/allClassNMS.cu
So the bottom line is, I think it is reasonable to say that NMS, without adjectives, should refer to the single class variant (category 2 above), but there isn't a consensus on what to call the third category one which this PR is about.
I think `all_class_non_maximum_surpression` or `per_class_non_maximum_surpression` are the most descriptive.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mbrookhart commented on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX / TF Combined NMS
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on pull request #7796:
URL: https://github.com/apache/tvm/pull/7796#issuecomment-813488548
Exciting! I won't review too much because it's still in draft, but the overall structure looks good to me. I agree that that relay-level loop was a problem since we couldn't parallelize it.
I'm seeing some removed attributes from the normal NMS op, I didn't add those as part of the ONNX NMS PR, I imagine they're used by other frontends?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org