Rotated bounding box NMS implementation for CPU#9450
Rotated bounding box NMS implementation for CPU#9450zy1git wants to merge 25 commits intopytorch:mainfrom
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/9450
Note: Links to docs will display an error until the docs builds have been completed. ❌ 2 New FailuresAs of commit aea073c with merge base d7400a3 ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
|
||
| auto ovr = single_box_iou_rotated<scalar_t>( | ||
| dets[i].data_ptr<scalar_t>(), dets[j].data_ptr<scalar_t>()); | ||
| if (ovr >= iou_threshold) { |
There was a problem hiding this comment.
Flagging that this is different from the iou threshold comparison we have in the non-rotated case:
See my other comment about unifying the implementation, which should resolve this as a consequence.
There was a problem hiding this comment.
Yes. It is resolved in the new commit after unifying the implementation.
| namespace { | ||
|
|
||
| template <typename scalar_t> | ||
| at::Tensor nms_rotated_cpu_kernel( |
There was a problem hiding this comment.
This is exactly the same implementation we already have for the non-rotated case, the only difference being the iou computation:
Could we consider fusing the two implementations, perhaps templating over the iou computation function?
There was a problem hiding this comment.
Yes, I had the same thought when I did the implementation. I wanted to stick to Detectron2's implementation in the first version to make sure it works correctly, then refactor. I have fused the two implementations in the new commit.
torchvision/ops/boxes.py
Outdated
| return torch.ops.torchvision.nms(boxes, scores, iou_threshold) | ||
|
|
||
|
|
||
| def nms_rotated(boxes: Tensor, scores: Tensor, iou_threshold: float) -> Tensor: |
There was a problem hiding this comment.
any reason to expose nms_rotated instead of just handling all this within a single nms function?
For iou, we chose not to expose iou_rotated at the Python layer.
There was a problem hiding this comment.
Thanks for pointing this out. I fixed this in the new commit.
…gorithm is standard TorchVision code; attribution already in box_iou_rotated_utils.h
torchvision/ops/boxes.py
Outdated
|
|
||
|
|
||
| def nms(boxes: Tensor, scores: Tensor, iou_threshold: float) -> Tensor: | ||
| def nms(boxes: Tensor, scores: Tensor, iou_threshold: float, fmt: str = "xyxy") -> Tensor: |
There was a problem hiding this comment.
Can we just infer the format base on the .shape attribute? If shape [-2] is 4 then it should be aligned, if 5 then it should be non-aligned?
I'm not sure, we'd have to verify that this actually doesn't break any other convension. Like, was shape[-2] == 5 even allowed before? If not, we're clear
There was a problem hiding this comment.
This is a good point. I think it should be shape[-1] rather than shape[-2]. I checked shape[-1] == 5 was not allowed before (the kernel enforces size(1) == 4). And the format is unique for dimension 4 and 5, so this works. I have implemented it in the new commit.
test/test_ops.py
Outdated
|
|
||
|
|
||
| class TestNMSRotated: | ||
| def _reference_horizontal_nms(self, boxes, scores, iou_threshold): |
There was a problem hiding this comment.
Use TestNMS._reference_nms instead, you might have to make it a class method instead of an instance method
test/test_ops.py
Outdated
| box_scores (N, 5): boxes in corner-form and probabilities. | ||
| (Note here 5 == 4 + 1, i.e., 4-dim horizontal box + 1-dim prob) |
There was a problem hiding this comment.
This will probably be resolved automatically when we use TestNMS._reference_nms, but flagging that this box_scores parameter doesn't exist.
There was a problem hiding this comment.
Good point. box_scores doesn't exist as a parameter. The docstring is incorrect. Detectron2 has the same issue: https://github.com/facebookresearch/detectron2/blob/main/tests/layers/test_nms_rotated.py#L46-L48
test/test_ops.py
Outdated
| m, n = len(keep1), len(keep2) | ||
|
|
||
| # edit distance with DP | ||
| f = [np.arange(n + 1), np.arange(n + 1)] |
There was a problem hiding this comment.
Got it. Will pay attention to this in future.
test/test_ops.py
Outdated
|
|
||
| keep_ref = self._reference_horizontal_nms(boxes, scores, iou) | ||
| keep = ops.nms(rotated_boxes, scores, iou) | ||
| assert self._nms_edit_distance(keep, keep_ref) <= 1 |
There was a problem hiding this comment.
Locally, I am able to use torch.assert_close(..., atol=0) on many random seeds, we should consider using this instead of edit distance.
There was a problem hiding this comment.
Fixed in the new commit.
test/test_ops.py
Outdated
| return boxes, scores | ||
|
|
||
| @pytest.mark.parametrize("iou", (0.2, 0.5, 0.8)) | ||
| def test_nms_rotated_0_degree(self, iou): |
There was a problem hiding this comment.
This is comparing our rotated implementation against _reference_horizontal_nms. We should also have a test that uses our non-rotated nms implementation as the reference.
There was a problem hiding this comment.
I agree. Fixed in the new commit.
test/test_ops.py
Outdated
| return torch.as_tensor(picked) | ||
|
|
||
| @staticmethod | ||
| def _nms_edit_distance(keep1, keep2): |
There was a problem hiding this comment.
Remove if we don't need it based on https://github.com/pytorch/vision/pull/9450/changes#r2995974410
There was a problem hiding this comment.
Removed in the new commit.
| auto inter = w * h; | ||
| auto ovr = inter / (iarea + areas[j] - inter); | ||
|
|
||
| auto ovr = iou_func.compare(j); |
There was a problem hiding this comment.
Nit: call this compute rather than compare
There was a problem hiding this comment.
fixed in the new commit.
|
|
||
| scalar_t ix1, iy1, ix2, iy2, iarea; | ||
|
|
||
| AABBIoU(const at::Tensor& dets) { |
There was a problem hiding this comment.
Call this NonRotatedIoU or AlignedIoU
There was a problem hiding this comment.
fixed in the new commit.
|
|
||
| RotatedIoU(const at::Tensor& dets) : dets_ptr(&dets) {} | ||
|
|
||
| int64_t cached_i; |
There was a problem hiding this comment.
fixed in the new commit.
test/test_ops.py
Outdated
| torch.testing.assert_close(iou_cpu, iou_cuda.cpu(), atol=1e-5, rtol=1e-5) | ||
|
|
||
|
|
||
| class TestNMSRotated: |
There was a problem hiding this comment.
Now that we are exposing this through the same nms() method, we should just move these tests within TestNMS.
There was a problem hiding this comment.
Addressed in the new commit.
test/test_ops.py
Outdated
| class TestNMS: | ||
| def _reference_nms(self, boxes, scores, iou_threshold): | ||
| @classmethod | ||
| def _reference_nms(cls, boxes, scores, iou_threshold): |
There was a problem hiding this comment.
now that we're moving the tests, we should rename this to _reference_aligned_nms or something like that
There was a problem hiding this comment.
Addressed in the new commit.
test/test_ops.py
Outdated
| boxes, scores = self._create_tensors(N) | ||
| rotated_boxes = torch.zeros(N, 5) | ||
| rotated_boxes[:, 0] = (boxes[:, 0] + boxes[:, 2]) / 2.0 | ||
| rotated_boxes[:, 1] = (boxes[:, 1] + boxes[:, 3]) / 2.0 | ||
| # Swap width and height for 90 degrees so reference horizontal NMS can be used | ||
| rotated_boxes[:, 2] = boxes[:, 3] - boxes[:, 1] | ||
| rotated_boxes[:, 3] = boxes[:, 2] - boxes[:, 0] | ||
| rotated_boxes[:, 4] = 90 |
There was a problem hiding this comment.
Instead of duplicating this logic everywhere, let's create aligned boxes, call a box format conversion function, and reset the column angle as desired. We probably want to do that in the _create_tensor() helper which hsould be renamed to _create_rotated_boxes()
test/test_ops.py
Outdated
| torch.testing.assert_close(keep, keep_non_rotated, atol=0, rtol=0) | ||
|
|
||
| @pytest.mark.parametrize("iou", (0.2, 0.5, 0.8)) | ||
| def test_nms_rotated_90_degrees(self, iou): |
There was a problem hiding this comment.
parametrize over the angle instead of duplicating the test N times. Also, it doesn't seem necessary for the angles to be a multiple of 90 for this to work, I think it'll work with any angle value, the reason everything works is because the angle is the same across boxes, but its actual value doesn't matter (I could be wrong, please double check)
There was a problem hiding this comment.
I parametrized over the angle in the new commit. However, I think we can only parametrize for 0, 90, and 180. For other angles like 45°, even though each box is rotated by the same angle, they rotate around their own centers (which are at different positions), so the overlapped area changes and we can't compare against axis-aligned NMS. For 90, we swap width and height so the rotated boxes cover the same region as the axis-aligned boxes, so the NMS are the same.
test/test_ops.py
Outdated
| torch.testing.assert_close(keep, keep_ref, atol=0, rtol=0) | ||
| keep_non_rotated = ops.nms(boxes, scores, iou) | ||
| torch.testing.assert_close(keep, keep_non_rotated, atol=0, rtol=0) | ||
|
|
There was a problem hiding this comment.
we should add tests for when the boxes have a different rotation angle, currently all tests have the same angle.
There was a problem hiding this comment.
I added test_nms_rotated_different_angles, which verifies basic NMS properties since we can't compare against axis-aligned NMS when boxes have different angles. And also I added test_nms_rotated_specific_angles to show that the NMS worked as expected for different specific angles. Please feel free to let me know what extra tests for different rotation angles you want me to add.
test/test_ops.py
Outdated
| torch.testing.assert_close(keep, keep_non_rotated, atol=0, rtol=0) | ||
|
|
||
| @pytest.mark.parametrize("iou", (0.2, 0.5, 0.8)) | ||
| def test_batched_nms_rotated_0_degree(self, iou): |
There was a problem hiding this comment.
Let's also parametrize the existing test_batched_nms_implementations test over the format (one rotated, one non-rotated).
There was a problem hiding this comment.
Addressed in the new commit
…different rotation angles per box
test/test_ops.py
Outdated
| backup = rotated_boxes.clone() | ||
| keep_non_rotated = ops.batched_nms(boxes, scores, idxs, iou) | ||
| keep = ops.batched_nms(rotated_boxes, scores, idxs, iou) | ||
| assert torch.allclose(rotated_boxes, backup) |
There was a problem hiding this comment.
fixed in the new commit.
test/test_ops.py
Outdated
| def test_nms_rotated_different_angles(self, iou): | ||
| torch.manual_seed(0) | ||
| N = 1000 | ||
| boxes, rotated_boxes, scores = self._create_rotated_boxes(N) |
There was a problem hiding this comment.
fixed by "_" in the new commit.
test/test_ops.py
Outdated
| torch.testing.assert_close(keep, keep_non_rotated, atol=0, rtol=0) | ||
|
|
||
| @pytest.mark.parametrize("iou", (0.2, 0.5, 0.8)) | ||
| def test_batched_nms_rotated_0_degree(self, iou): |
There was a problem hiding this comment.
sounds like this could be tested on 90 and 180 as well. Consider parametrizing with test_nms_rotated if it doesn't complicate the code too much.
There was a problem hiding this comment.
Addressed in the new commit.
test/test_ops.py
Outdated
| if angle == 90: | ||
| rotated_boxes[:, 2] = cxcywh[:, 3] | ||
| rotated_boxes[:, 3] = cxcywh[:, 2] | ||
| else: | ||
| rotated_boxes[:, 2] = cxcywh[:, 2] | ||
| rotated_boxes[:, 3] = cxcywh[:, 3] |
There was a problem hiding this comment.
This trick should be explained. I suspect we'll want to move it out of the _create_rotated_boxes as it's mainly used to make the existing test_nms_rotated work.
There was a problem hiding this comment.
Addressed in the new commit.
| (*dets_ptr)[i].template data_ptr<scalar_t>(), | ||
| (*dets_ptr)[j].template data_ptr<scalar_t>()); |
There was a problem hiding this comment.
can you explain the .template syntax?
There was a problem hiding this comment.
I added .template to fix a CI failure we hit on the macOS build. It explicitly indicates that data_ptr<scalar_t> is a template method. Without it, the compiler can't tell whether data_ptr<scalar_t> is a template method call or a less-than comparison.
…ve boxes in test_nms_rotated_different_angles function
Summary:
Implemented rotated box NMS (Non-Maximum Suppression) for CPU, adapted from Detectron2's nms_rotated implementation. The NMS algorithm is identical to standard NMS — sort by scores, suppress overlapping boxes — but uses
single_box_iou_rotatedfor IoU computation instead of axis-aligned intersection. The public API follows the existing nms op pattern in TorchVision.Test Plan:
Added TestNMSRotated test class adapted from Detectron2's test suite:
0° rotation test: rotated NMS with angle=0 should match reference horizontal NMS (IoU thresholds 0.2, 0.5, 0.8)
90° rotation test: rotated NMS with angle=90 and swapped width/height should match reference horizontal NMS
180° rotation test: rotated NMS with angle=180 should match reference horizontal NMS
Run
pytest test/test_ops.py::TestNMSRotated -vAll tests pass locally.