[GitHub] [tvm] trevor-m commented on pull request #7796: [TOPI, Relay] A new NMS op variant for ONNX NMS / TF Combined NMS

[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