Queries regarding Non Max Suppression implementation

[dpanicka]

Looking through the NMS implementation, I have a couple of queries based on the documentation from Tensorflow V4 and ONNX NMS.

• Why is the box with minimum scores calculated across all classes to fill up to maxOutputPerBatch here? Do we need to fill up to the maximum output possible?
• numDetected is only applicable for Tensorflow V4 and it is supposed to be a 0-D tensor. Why is the following done for both ONNX and Tensorflow here?
• In the tests for NMS, batches are being ignored which leads to some values not being checked, here. Is this intentional?

It would be good to discuss these with the original author, @ayermolo . Thanks!

[ayermolo]

1. Well NMS outputs indices (batch, box, class). Usually then it goes through post processing (pp) subgraph with bunch of select/gathers. With ONNX nms PP doesn't know how many are valid so gather will access all indices. If we don't fill it it's either going to be un-initialized or 0. With un-initialed it can lead to unintended behavior and access some random piece of memory in gather. If 0th box happens to be best Score then it will dominate after topK. With min box all the entries after maxOutputPerBatch will usually be disregarded or show up at the end.

2. It was to avoid broadcast. In networks I have seen it was usually replicated for layers that do selection. In ONX NMS it's not used anyway. I guess it can be gated by V4 flag.

3. I think you are referring to ones that use gather to extract values right? The graph it self is constructed with batch dim 1. There are other tests that instantiate testNonMaxSuppression, that use batch > 1.

[dpanicka]

Thank you for your comments!

1. That's a good point regarding gather and the 0th box. If we could avoid unnecessary computation, that would be best since we only care about the valid boxes anyway. Will have a think about it.

2. OK.

3. I meant the testNonMaxSuppression without gather, here. The loop goes up to maxOutputPerClass, and that would only check the first maxOutputPerClass number of results without considering batches.

I'll try to put up a patch and it would be great if you could review it.

[ayermolo]

1. Yeah it's a problem. There is a fundamental issue that in GLOW dimensions are static, and NMS output is dynamic. So if you construct PP you have to assume that max is detected.
2. Ah I see. Yeah should be extended to number of batch dimensions. I think all the tests use only one class.

Sure, although I don't have permissions to approve. Someone from FB team will need to do that.