Distance IoU #5786

yassineAlouini · 2022-04-07T14:21:45Z

In this PR, I implement the distance IoU and associated loss as described from this paper: https://arxiv.org/abs/1911.08287.
The implementation is inspired and adapted from here https://github.com/facebookresearch/detectron2/blob/dfe8d368c8b7cc2be42c5c3faf9bdcc3c08257b1/detectron2/layers/losses.py#L5 and from the implemented giou.

…ests).

yassineAlouini · 2022-04-07T14:29:47Z

I have to improve the tests a bit. I have also some refactoring ideas in mind but it is better to leave those for a new PR.

datumbox · 2022-04-07T14:30:38Z

@yassineAlouini Thanks for the contribution. I know your PR is still draft but it's worth looking at the comments at #5776 since many of them are relevant to you too.

yassineAlouini · 2022-04-07T15:04:49Z

@yassineAlouini Thanks for the contribution. I know your PR is still draft but it's worth looking at the comments at #5776 since many of them are relevant to you too.

Indeed. I am already using some of the first feedbacks and I will look for new ones. 👌

… performance drop.

torchvision/ops/__init__.py

torchvision/ops/giou_loss.py

…from cIoU ones).

yassineAlouini · 2022-04-13T15:00:01Z

@oke-aditya @abhi-glitchhg I have added more tests highly inspired from cIoU and fixed the ones I have added already.
I think it is a good time to give this PR reviews if you can and I will then take into account your suggestions. Thanks in advance. 👍

test/test_ops.py

oke-aditya · 2022-04-13T18:13:06Z

test/test_ops.py

+    @staticmethod
+    def assert_distance_iou_loss(box1, box2, expected_output, dtype, device, reduction="none"):
+        output = ops.distance_box_iou_loss(box1, box2, reduction=reduction)
+        expected_output = torch.tensor(expected_output, dtype=dtype, device=device)


I think this will automatically take the dtype, by passing dtype as function arg. It will also get parameterized,

Suggested change

expected_output = torch.tensor(expected_output, dtype=dtype, device=device)

expected_output = torch.tensor(expected_output, device=device)

Notice that

https://github.com/pytorch/vision/pull/5792/files#diff-d183f2afc51d6a59bc70094e8f476d2468c45e415500f6eb60abad955e065156R1587

this works!

Not exactly the same. I made the nested function into a staticmethod so it doesn't have access to the same scope.

Yeah, but why is it a static method in the first place? IIUC, we are only using inside test_distance_iou_loss, correct? If yes, we can simply inline it, which also removes the need to pass the device and dtype.

oke-aditya · 2022-04-13T18:41:43Z

test/test_ops.py

@@ -1258,6 +1258,85 @@ def test_giou_jit(self) -> None:
        self._run_jit_test([[0, 0, 100, 100], [0, 0, 50, 50], [200, 200, 300, 300]])


+class TestDistanceBoxIoU(BoxTestBase):
+    def _target_fn(self) -> Tuple[bool, Callable]:


A general question to torchvision maintainers
Do we type hint in tests?
It's not bad to type hint. But it isn't something which we follow either 😅

cc @pmeier @datumbox

No, we don't. See #5563 (comment). The reason is that without also checking the tests with mypy they might go out of date and that is usually more harmful than not having them at all.

I am removing the type hints then, thanks for the link @pmeier. 👍

yassineAlouini · 2022-04-14T09:41:10Z

@oke-aditya @abhi-glitchhg @pmeier I have a question regarding float16 input to the distance_box_iou_loss function:
is it required to also have the output as a float16 or is float32 enough?

If float16 is required, what is the best way to achieve this? Is disabling the autocast a good thing to do or is there a better way. Thanks for any help.

datumbox · 2022-04-26T11:12:39Z

torchvision/ops/diou_loss.py

+        batch_boxes2 = boxes2.unsqueeze(0)
+        diou = distance_box_iou(batch_boxes1, batch_boxes2, eps)[0, 0]
+    else:
+        diou = distance_box_iou(boxes1, boxes2, eps)[0]


@yassineAlouini I'm not sure this approach is equivalent to what we had earlier. Please correct me if I'm wrong but I understand that distance_box_iou() does NxM pairwise comparisons while here we just want the Nx1 comparisons. This can be given by using the diagonal but this should be extremely slow as we would be throwing away the majority of operations.

What I had in mind is try to refactor the code at ops.boxes so that we can share some of the estimations. Thoughts?

cc @abhi-glitchhg because you follow a similar approach on the other PR.

BTW if you both prefer to revert to your earlier versions of the code (which didn't reuse ops.boxes) and tackle this on separate future PRs, I'm happy to go down that route. The PRs for cIoU and dIoU has been dragging for a while and I appreciate that this can become frustrating at one point. Let me know your preference so that we make this a more enjoyable experience for both of you. Thanks a bunch for your work so far. :)

@datumbox , I agree with your concerns. This is not computationally efficient.

BTW if you both prefer to revert to your earlier versions of the code (which didn't reuse ops.boxes) and tackle this on separate future PRs,

Yeah, sounds good. :)

@yassineAlouini I'm not sure this approach is equivalent to what we had earlier. Please correct me if I'm wrong but I understand that distance_box_iou() does NxM pairwise comparisons while here we just want the Nx1 comparisons. This can be given by using the diagonal but this should be extremely slow as we would be throwing away the majority of operations.

What I had in mind is try to refactor the code at ops.boxes so that we can share some of the estimations. Thoughts?

cc @abhi-glitchhg because you follow a similar approach on the other PR.

That's a very good point and I might have introduced a bug by going quickly on my refactor, so thanks for pointing this out.

I can revert to previous code or keep working on this here (in this PR), both work for me. 👌

Thanks for the flexibility! Let's revert and use the previously vetted code on the loss. We can investigate refactoring all losses to share code with ops.boxes on separate PRs as you originally suggested.

Sounds great! I have also found a fix for the torch.half casting issue but it requires removing the _upcast of the boxes. I have left a TODO where I have removed the casting.

There are many options:

either I remove the torch.half tests for now.

remove the _upcast but could get an overflow error as you have mentioned above.

keep investigating to find a fix for all the problems at once.

What are your thoughts @datumbox? 🤔

@yassineAlouini From what I see you've reverted the code that reused estimations from ops.boxes and now use the a modified version of the loss at Detectron2. BTW we should add a reference in the source code similar to this, to indicate the original source.

Concerning the casting question, note that in ops.boxes the _upcast method is primarily there to handle overflows of low precision values that might overflow. The main concern is integers (because those methods need to support them). In the case of losses, the _upcast method works a bit different as it converts everything to floats (we don't handle integers in losses). This is crucial because some of the area estimation operations will overflow in torch.float16.

>>> torch.finfo(torch.float16) finfo(resolution=0.001, min=-65504, max=65504, eps=0.000976562, tiny=6.10352e-05, dtype=float16)

So I think the safe thing to do here is to follow the same approach as in gIoU and upcast. I think we should move the method _upcast to _utils so it can be shared. This is going to be useful also for the cIoU PR that @abhi-glitchhg is working on.

cc @fmassa for visibility in case I stated something incorrect here.

…ed a bug and can be slow.

abhi-glitchhg · 2022-04-26T18:39:29Z

torchvision/ops/boxes.py

+    # centers of boxes
+    x_p = boxes1[:, None, :2].sum() / 2
+    y_p = boxes1[:, None, 2:].sum() / 2
+    x_g = boxes2[:, :2].sum() / 2
+    y_g = boxes2[:, 2:].sum() / 2


Hey @yassineAlouini , I think there is a problem with this implementation. The calculation of centre of boxes is not correct acc to me. We should be adding up only x1 x2 and y1 y2, ref .
But in current implementation, we are adding x1,y1 and x2, y2. (BBox shape is in form [x1,y1,x2,y2]) .

This can also be checked by calculating distance_box_iou_loss and distance_box_iou on a sample tensors.

import torch from torchvision.ops import distance_box_iou, distance_box_iou_loss box1 = torch.tensor([[-1, -1, 1, 1]], ) box2 = torch.tensor([[0, 0, 1, 1]],) 1-distance_box_iou(box1, box2)[0] == distance_box_iou_loss(box1, box2)

Last statement returns False. Ideally it should return True.

I suggest you to do following changes.

Suggested change

# centers of boxes

x_p = boxes1[:, None, :2].sum() / 2

y_p = boxes1[:, None, 2:].sum() / 2

x_g = boxes2[:, :2].sum() / 2

y_g = boxes2[:, 2:].sum() / 2

# centers of boxes

x_p = (boxes1[:, 0] + boxes1[:, 2]) / 2

y_p = (boxes1[:, 1] + boxes1[:, 3]) / 2

x_g = (boxes2[:, 0] + boxes2[:, 2]) / 2

y_g = (boxes2[:, 1] + boxes2[:, 3]) / 2

@datumbox,
Please correct me if I'm wrong.
Thanks.

Good catch. I haven't yet reviewed the correctness of the implementation as we still discuss the structure/API.

I think that's probably a typo and @yassineAlouini intended to write something like:

x_p = boxes1[:, 0::2].sum() / 2 y_p = boxes1[:, 1::2].sum() / 2 ...

Yes indeed, I think I went too quickly over this and thought that the bounding box was in the x1x2y1y2 format. Thanks for pointing this out and your suggestions. 👍

yassineAlouini · 2022-05-02T07:12:25Z

@datumbox I see that the cIoU metric has been merged. I will take inspiration to fix remaining issues today hopefully. 👌

yassineAlouini · 2022-05-02T09:02:20Z

@datumbox @pmeier I made the code iso with the cIoU one. However, I have noticed two things:

some imports are missing in the __all__ for cIoU and have fixed them.
the torch.half nightmarish test 😁 is still not working. I thought that the cIoU code found a clever fix but it seems that it is missing the dtype setup (I have put a TODO so that we don't forget it) in the appropriate test and so once I add it, it fails as well. I think it is best to fix this in the next PR (where I will refactor some of the tests).

datumbox

@yassineAlouini Apologies for the delay. I was OOO last week.

I've reviewed the code of the implementation and it looks good to me! There was a code attribution missing but I pushed directly into your branch to avoid the back and forth.

In addition to the review, I verified that the two implementations for boxes and losses return the same result by running:

import torch

from torchvision.ops.boxes import distance_box_iou
from torchvision.ops.diou_loss import distance_box_iou_loss

def random_box(canvas_size):
    x1y1 = torch.rand((1, 2)) * canvas_size
    wh = torch.rand((1, 2)) * canvas_size
    x2y2 = (x1y1 + wh).clamp(0, canvas_size)
    return torch.cat((x1y1, x2y2), axis=1)


canvas_size = 1000


for _ in range(10000):
    box1 = random_box(canvas_size)
    box2 = random_box(canvas_size)

    v1 = 1 - distance_box_iou(box1, box2)
    v2 = distance_box_iou_loss(box1, box2)

    torch.testing.assert_close(v1[0], v2, rtol=0, atol=1e-6)

@pmeier Any blocking changes required on the testing side? If not I recommend merging and do a deep cleaning between the g/c/d-iou.

@yassineAlouini @abhi-glitchhg @oke-aditya Anyone interested in doing this cleanup?

oke-aditya · 2022-05-09T11:53:45Z

test/test_ops.py

@@ -1676,6 +1767,7 @@ def test_ciou_loss(self, dtype, device):
        def assert_ciou_loss(box1, box2, expected_output, reduction="none"):

            output = ops.complete_box_iou_loss(box1, box2, reduction=reduction)
+            # TODO: When passing the dtype, the torch.half test doesn't pass...


Is it still valid?

Can we provide a bit more info on what doesn't pass here and what's exactly the issue?

@oke-aditya I think so.

@datumbox I read the cIoU code since it was passing the torch.half tests and I found out that the dtype wasn't passed so the test wasn't correct for torch.half. For now, I have removed the dtype to have the same code as cIoU but I think we should investigate this further (or maybe we can't do anything since we use the _upcast function? 🤔). Let me know if this clear enough. I can provide more details.

oke-aditya · 2022-05-09T11:55:24Z

torchvision/ops/diou_loss.py

+from ..utils import _log_api_usage_once
+from .boxes import _upcast
+
+


Commenting above the function might be better? As comments in the function call will be a small execution of commented code everytime we call code? (Is there a subtle performance difference? Not sure but always had this doubt)

oke-aditya · 2022-05-09T11:55:48Z

torchvision/ops/diou_loss.py

+        https://arxiv.org/abs/1911.08287
+    """
+
+    # Original Implementation : https://github.com/facebookresearch/detectron2/blob/main/detectron2/layers/losses.py


I meant this comment

oke-aditya · 2022-05-09T12:03:09Z

I can clean up. But @yassineAlouini feel free if you want to have a go :)

pmeier

Testing LGTM, thanks @yassineAlouini!

datumbox · 2022-05-09T13:58:33Z

@oke-aditya Here is the ticket: #5976. Feel free to assign it to yourself or comment you want it so that I can assign it to you (Github won't let me do it now for some reason).

pmeier · 2022-05-09T14:02:11Z

Github won't let me do it now for some reason).

Unless you they have commented on the issue, you can only assign issues to people that have a least collaborator status.

yassineAlouini · 2022-05-10T09:13:37Z

I can clean up. But @yassineAlouini feel free if you want to have a go :)

Thanks @oke-aditya for the review. You can work on the #5976 ticket. I can help with any additional/related tasks if needed. 👌

oke-aditya · 2022-05-10T09:21:17Z

@yassineAlouini Feel free to review #5979

Summary: * [FEAT] Add distance IoU and distance IoU loss + some tests (WIP for tests). * [FIX] Remove URL from docstring + remove assert since it causes a big performance drop. * [FIX] eps isn't None. * [TEST] Update existing box dIoU test + add dIoU loss tests (inspired from cIoU ones). * [ENH] Some pre-commit fixes + remove print + mypy. * [ENH] Pass the device in the assertion for the dIoU loss test. * [FIX] Remove type hints from the dIoU box test. * [ENH] Refactor box and loss for dIoU functions + fix half tests. * [FIX] Precommits fix. * [ENH] Some improvement for the distance IoU tests thanks to code review. * [ENH] Upcast in distance boxes computation to avoid overflow. * [ENH] Revert the refactor of distance IoU loss back since it introduced a bug and can be slow. * Precommit fix. * [FIX] Few changes introduced by merge conflict. * Add code reference * Fix test Reviewed By: YosuaMichael Differential Revision: D36281596 fbshipit-source-id: 70e5102ec6fae9c9795d1895911f94f0a74e42f8 Co-authored-by: Vasilis Vryniotis <datumbox@users.noreply.github.com>

yassineAlouini · 2022-05-13T14:41:36Z

@yassineAlouini Feel free to review #5979

I will, thanks for the suggestion @oke-aditya. 👌

[FEAT] Add distance IoU and distance IoU loss + some tests (WIP for t…

135763c

…ests).

yassineAlouini changed the title ~~[FEAT] Add distance IoU and distance IoU loss + some tests (WIP for t…~~ Distance IoU Apr 7, 2022

facebook-github-bot added the cla signed label Apr 7, 2022

datumbox mentioned this pull request Apr 7, 2022

[RFC] Batteries Included - Phase 2 #5410

Closed

24 tasks

yassineAlouini mentioned this pull request Apr 7, 2022

[RFC] Loss Functions in Torchvision #2980

Open

20 tasks

datumbox added module: ops topic: object detection new feature labels Apr 7, 2022

Yassine Alouini added 2 commits April 7, 2022 17:16

[FIX] Remove URL from docstring + remove assert since it causes a big…

ec599d2

… performance drop.

[FIX] eps isn't None.

41703e6

oke-aditya reviewed Apr 7, 2022

View reviewed changes

torchvision/ops/__init__.py Show resolved Hide resolved

abhi-glitchhg reviewed Apr 10, 2022

View reviewed changes

torchvision/ops/giou_loss.py Show resolved Hide resolved

Yassine Alouini added 2 commits April 13, 2022 16:51

[TEST] Update existing box dIoU test + add dIoU loss tests (inspired …

51616ed

…from cIoU ones).

Merge branch 'main' into dIoU

ee37c8d

yassineAlouini marked this pull request as ready for review April 13, 2022 14:58

oke-aditya reviewed Apr 13, 2022

View reviewed changes

test/test_ops.py Outdated Show resolved Hide resolved

Yassine Alouini added 2 commits April 13, 2022 17:24

[ENH] Some pre-commit fixes + remove print + mypy.

7631ab7

Merge branch 'dIoU' of github.com:yassineAlouini/vision-1 into dIoU

b744d6d

abhi-glitchhg reviewed Apr 13, 2022

View reviewed changes

test/test_ops.py Outdated Show resolved Hide resolved

[ENH] Pass the device in the assertion for the dIoU loss test.

8ceffcc

oke-aditya reviewed Apr 13, 2022

View reviewed changes

Merge branch 'main' into dIoU

bc65b83

Yassine Alouini added 2 commits April 14, 2022 11:51

[FIX] Remove type hints from the dIoU box test.

a4e58b7

Merge branch 'dIoU' of github.com:yassineAlouini/vision-1 into dIoU

4ba5cdc

datumbox reviewed Apr 26, 2022

View reviewed changes

datumbox mentioned this pull request Apr 26, 2022

use generalised_box_iou function to calculate giou_loss #5877

Closed

Yassine Alouini added 2 commits April 26, 2022 15:11

[ENH] Revert the refactor of distance IoU loss back since it introduc…

d7baa67

…ed a bug and can be slow.

Precommit fix.

4213ee4

abhi-glitchhg reviewed Apr 26, 2022

View reviewed changes

datumbox mentioned this pull request Apr 28, 2022

Added CIOU loss function #5776

Merged

Merge main and fix conflicts + make code iso with cIoU.

1a2d6ab

Yassine Alouini and others added 2 commits May 2, 2022 11:10

[FIX] Few changes introduced by merge conflict.

2856947

Add code reference

3a9d3d7

datumbox approved these changes May 9, 2022

View reviewed changes

oke-aditya reviewed May 9, 2022

View reviewed changes

Merge branch 'main' into dIoU

13fa495

pmeier approved these changes May 9, 2022

View reviewed changes

Fix test

1b2f1e6

datumbox mentioned this pull request May 9, 2022

Clean up Ops Box and Losses implementations #5976

Closed

Merge branch 'main' into dIoU

ab44428

datumbox merged commit 1ae3829 into pytorch:main May 9, 2022

yassineAlouini deleted the dIoU branch May 10, 2022 09:14

abhi-glitchhg mentioned this pull request Jul 26, 2022

distance_box_iou() and complete_box_iou() don't work if both sets don't have the same number of boxes #6317

Closed

	expected_output = torch.tensor(expected_output, dtype=dtype, device=device)
	expected_output = torch.tensor(expected_output, device=device)

		from ..utils import _log_api_usage_once
		from .boxes import _upcast

Distance IoU #5786

Distance IoU #5786

Conversation

yassineAlouini commented Apr 7, 2022 • edited Loading

yassineAlouini commented Apr 7, 2022

datumbox commented Apr 7, 2022

yassineAlouini commented Apr 7, 2022

yassineAlouini commented Apr 13, 2022

Choose a reason for hiding this comment

yassineAlouini Apr 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yassineAlouini commented Apr 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhi-glitchhg Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yassineAlouini Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yassineAlouini commented May 2, 2022

yassineAlouini commented May 2, 2022 • edited Loading

datumbox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yassineAlouini May 10, 2022 • edited Loading

Choose a reason for hiding this comment

oke-aditya May 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oke-aditya commented May 9, 2022

pmeier left a comment

Choose a reason for hiding this comment

datumbox commented May 9, 2022

pmeier commented May 9, 2022

yassineAlouini commented May 10, 2022

oke-aditya commented May 10, 2022

yassineAlouini commented May 13, 2022

yassineAlouini commented Apr 7, 2022 •

edited

Loading

yassineAlouini Apr 14, 2022 •

edited

Loading

yassineAlouini commented Apr 14, 2022 •

edited

Loading

abhi-glitchhg Apr 26, 2022 •

edited

Loading

yassineAlouini Apr 26, 2022 •

edited

Loading

yassineAlouini commented May 2, 2022 •

edited

Loading

yassineAlouini May 10, 2022 •

edited

Loading

oke-aditya May 9, 2022 •

edited

Loading