[GitHub] [tvm] masahi edited a comment on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX / TF Combined NMS

[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