Add onnx.NonMaxSuppression #436
Conversation
This patch adds one basic test that passes in identical boxes, and one test that passes in boxes that use center point.
|
Looks like I can't add reviewers, @zjgarvey Could you please take a look? Thanks! |
| class NonMaxSuppressionTestBasic(BuildAModel): | ||
| def construct_i_o_value_info(self): | ||
| self.input_vi = [ | ||
| make_tensor_value_info("boxes", TensorProto.FLOAT, [1, 10, 4]), |
There was a problem hiding this comment.
FYI there are existing tests for this operator and similar inputs in both SHARK-TestSuite and iree-test-suites, both derived from the upstream tests at https://github.com/onnx/onnx/blob/main/onnx/backend/test/case/node/nonmaxsuppression.py
- upstream generated test case 1: https://github.com/onnx/onnx/tree/main/onnx/backend/test/data/node/test_nonmaxsuppression_identical_boxes
- upstream generated test case 2: https://github.com/onnx/onnx/tree/main/onnx/backend/test/data/node/test_nonmaxsuppression_center_point_box_format
Generated tests are defined in SHARK-TestSuite here:
... and in iree-test-suites here:
There was a problem hiding this comment.
Thanks for pointing this out. Does that mean I should instead just update the expected failure list if llvm/torch-mlir#3976 is merged?
There was a problem hiding this comment.
I expect that when IREE pulls in your commit to torch-mlir, the CI workflow will "fail" (unexpectedly passing / XPASS) and then we'll remove these XFAILS:
- https://github.com/iree-org/iree/blob/4a7af8728126c722d4b293af1bc8bf21b732164a/tests/external/iree-test-suites/onnx_ops/onnx_ops_cpu_llvm_sync.json#L112
- https://github.com/iree-org/iree/blob/4a7af8728126c722d4b293af1bc8bf21b732164a/tests/external/iree-test-suites/onnx_ops/onnx_ops_gpu_rocm_rdna3.json#L107
- https://github.com/iree-org/iree/blob/4a7af8728126c722d4b293af1bc8bf21b732164a/tests/external/iree-test-suites/onnx_ops/onnx_ops_gpu_vulkan.json#L149
Similar to how iree-org/iree#19659 fixed some test_maxpool_2d_ceil_output_size_reduce_by_one tests.
There was a problem hiding this comment.
Adding new hand-written / Python tests to alt_e2eshark/onnx_tests/operators/ is okay... especially as a learning exercise to see how all the pieces fit together when working with a framework and the underlying tools directly. I personally would suggest against making such hand-written tests cover all cases exhaustively though, since that duplicates work that is already done upstream and in generated test suites and introduces more code to maintain.
When center_point_box=1, the supplied boxes come with a format of [x_center, y_center, width, height], this patch converts the format into [x1, y1, x2, y2] format before they are consumed. The e2e test is added in nod-ai/SHARK-TestSuite#436
This patch adds one basic test that passes in identical boxes, and one test that passes in boxes that use center point.
The center_point_box=1 support is added in llvm/torch-mlir#3976