Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds torchscript Compatibility to box_convert #2737

Merged
merged 10 commits into from
Oct 7, 2020
22 changes: 11 additions & 11 deletions test/test_ops.py
Original file line number Diff line number Diff line change
Expand Up @@ -727,20 +727,20 @@ def test_bbox_xywh_cxcywh(self):
self.assertEqual(box_xywh.dtype, box_tensor.dtype)
assert torch.all(torch.eq(box_xywh, box_tensor)).item()

# def test_bbox_convert_jit(self):
# box_tensor = torch.tensor([[0, 0, 100, 100], [0, 0, 0, 0],
# [10, 15, 30, 35], [23, 35, 93, 95]], dtype=torch.float)
def test_bbox_convert_jit(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if it would be more simple to check scripted function in all your test_bbox_*:

scripted_fn = torch.jit.script(ops.box_convert)

for fn in [ops.box_convert, scripted_fn]:
    # ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible, this PR is open for a lot of refactoring 👍 .

box_tensor = torch.tensor([[0, 0, 100, 100], [0, 0, 0, 0],
[10, 15, 30, 35], [23, 35, 93, 95]], dtype=torch.float)

# scripted_fn = torch.jit.script(ops.box_convert)
# TOLERANCE = 1e-3
scripted_fn = torch.jit.script(ops.box_convert)
TOLERANCE = 1e-3

# box_xywh = ops.box_convert(box_tensor, in_fmt="xyxy", out_fmt="xywh")
# scripted_xywh = scripted_fn(box_tensor, 'xyxy', 'xywh')
# self.assertTrue((scripted_xywh - box_xywh).abs().max() < TOLERANCE)
box_xywh = ops.box_convert(box_tensor, in_fmt="xyxy", out_fmt="xywh")
scripted_xywh = scripted_fn(box_tensor, 'xyxy', 'xywh')
self.assertTrue((scripted_xywh - box_xywh).abs().max() < TOLERANCE)

# box_cxcywh = ops.box_convert(box_tensor, in_fmt="xyxy", out_fmt="cxcywh")
# scripted_cxcywh = scripted_fn(box_tensor, 'xyxy', 'cxcywh')
# self.assertTrue((scripted_cxcywh - box_cxcywh).abs().max() < TOLERANCE)
box_cxcywh = ops.box_convert(box_tensor, in_fmt="xyxy", out_fmt="cxcywh")
scripted_cxcywh = scripted_fn(box_tensor, 'xyxy', 'cxcywh')
self.assertTrue((scripted_cxcywh - box_cxcywh).abs().max() < TOLERANCE)


class BoxAreaTester(unittest.TestCase):
Expand Down
6 changes: 3 additions & 3 deletions torchvision/ops/_box_convert.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ def _box_xyxy_to_xywh(boxes: Tensor) -> Tensor:
boxes (Tensor[N, 4]): boxes in (x, y, w, h) format.
"""
x1, y1, x2, y2 = boxes.unbind(-1)
x2 = x2 - x1 # x2 - x1
y2 = y2 - y1 # y2 - y1
boxes = torch.stack((x1, y1, x2, y2), dim=-1)
w = x2 - x1 # x2 - x1
h = y2 - y1 # y2 - y1
boxes = torch.stack((x1, y1, w, h), dim=-1)
return boxes
14 changes: 12 additions & 2 deletions torchvision/ops/boxes.py
Original file line number Diff line number Diff line change
Expand Up @@ -167,25 +167,35 @@ def box_convert(boxes: Tensor, in_fmt: str, out_fmt: str) -> Tensor:
boxes_xyxy = _box_xywh_to_xyxy(boxes)
if out_fmt == "cxcywh":
boxes_converted = _box_xyxy_to_cxcywh(boxes_xyxy)
else:
raise ValueError("Unsupported Boundig Box Conversions for given in_fmt and out_fmt")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to add tests to improve the coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is assert statement before these ValueError codeblocks. So the test will never reach this stage. Should I remove the assert statements?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, please

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added 👍


elif in_fmt == "cxcywh":
boxes_xyxy = _box_cxcywh_to_xyxy(boxes)
if out_fmt == "xywh":
boxes_converted = _box_xyxy_to_xywh(boxes_xyxy)

else:
raise ValueError("Unsupported Boundig Box Conversions for given in_fmt and out_fmt")
else:
raise ValueError("Unsupported Boundig Box Conversions for given in_fmt and out_fmt")
# convert one to xyxy and change either in_fmt or out_fmt to xyxy
else:
if in_fmt == "xyxy":
if out_fmt == "xywh":
boxes_converted = _box_xyxy_to_xywh(boxes)
elif out_fmt == "cxcywh":
boxes_converted = _box_xyxy_to_cxcywh(boxes)
else:
raise ValueError("Unsupported Boundig Box Conversions for given in_fmt and out_fmt")
elif out_fmt == "xyxy":
if in_fmt == "xywh":
boxes_converted = _box_xywh_to_xyxy(boxes)
elif in_fmt == "cxcywh":
boxes_converted = _box_cxcywh_to_xyxy(boxes)

else:
raise ValueError("Unsupported Boundig Box Conversions for given in_fmt and out_fmt")
else:
raise ValueError("Unsupported Boundig Box Conversions for given in_fmt and out_fmt")
return boxes_converted


Expand Down