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

Fix d/c IoU for different batch sizes #6338

Merged
merged 13 commits into from Aug 2, 2022
Merged

Conversation

oke-aditya
Copy link
Contributor

@oke-aditya oke-aditya commented Jul 31, 2022

Closes #6317

I hope I have done it correct.

Also I added tests to check both the cases M>N and N>M, this needed a small tests refactor. Let's be on safe side 😄

cc @datumbox @yassineAlouini

@oke-aditya oke-aditya changed the title Fix g/c IoU for different batch sizes Fix d/c IoU for different batch sizes Jul 31, 2022
torchvision/ops/boxes.py Outdated Show resolved Hide resolved
torchvision/ops/boxes.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@oke-aditya oke-aditya left a comment

Choose a reason for hiding this comment

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

@datumbox Plz verify these changes, I think I have done it correctly.

torchvision/ops/boxes.py Outdated Show resolved Hide resolved
test/test_ops.py Outdated Show resolved Hide resolved
torchvision/ops/boxes.py Outdated Show resolved Hide resolved
@fmassa
Copy link
Member

fmassa commented Aug 1, 2022

Hi,

I've implemented D-IoU a while back (when I was working on DETR), and it had the same API as generalized_iou. Here is the implementation:

# modified from torchvision to also return the union
def box_iou(boxes1, boxes2):
    area1 = box_area(boxes1)
    area2 = box_area(boxes2)

    lt = torch.max(boxes1[:, None, :2], boxes2[:, :2])  # [N,M,2]
    rb = torch.min(boxes1[:, None, 2:], boxes2[:, 2:])  # [N,M,2]

    wh = (rb - lt).clamp(min=0)  # [N,M,2]
    inter = wh[:, :, 0] * wh[:, :, 1]  # [N,M]

    union = area1[:, None] + area2 - inter

    iou = inter / union
    return iou, union


def distance_iou(boxes1, boxes2):
    iou, union = box_iou(boxes1, boxes2)

    lt = torch.min(boxes1[:, None, :2], boxes2[:, :2])
    rb = torch.max(boxes1[:, None, 2:], boxes2[:, 2:])

    wh = (rb - lt).clamp(min=0)  # [N,M,2]
    c2 = wh[..., 0] ** 2 + wh[..., 1] ** 2

    b1 = (boxes1[:, 2:] + boxes1[:, :2]) / 2
    b2 = (boxes2[:, 2:] + boxes2[:, :2]) / 2
    # rho2 = torch.cdist(b1, b2, p=2) ** 2
    rho2 = ((b1[:, None, :] - b2[None, :, :]) ** 2).sum(-1)

    return 1 - iou + rho2 / c2

Note that I commented out the torch.cdist line in the code and replaced it with the line below because using cdist would cause instabilities. I don't exactly remember what type of instabilities was there (and maybe this was fixed with a newer version of PyTorch already).

Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

Changes LGTM!

One thing that I might have missed is that it would be good to have a sanity test that compares that the result of a [N, 4] by [M, 4] tensor is actually equal to performing multiple calls to [1, 4] and [1, 4] tensors. This way we ensure that we don't have the matrix transposed in any way

torchvision/ops/boxes.py Show resolved Hide resolved
@datumbox
Copy link
Contributor

datumbox commented Aug 1, 2022

There seems to be a bug on the original implementation other than what you try to fix. As a result the expected values were wrong. To prove this, take @fmassa's script from #6338 (comment) and run the current test cases:

def distance_box_iou(boxes1, boxes2):
    return 1 - distance_iou(boxes1, boxes2)

box1 = torch.tensor([[0, 0, 100, 100], [0, 0, 50, 50], [200, 200, 300, 300], [0, 0, 25, 25]])
box2 = torch.tensor([[0, 0, 100, 100], [0, 0, 50, 50], [200, 200, 300, 300]])
print(distance_box_iou(box1, box2))

The output is:

tensor([[ 1.0000,  0.1875, -0.4444],
        [ 0.1875,  1.0000, -0.5625],
        [-0.4444, -0.5625,  1.0000],
        [-0.0781,  0.1875, -0.6267]])

Which is different from the expected values on main:

[[1.0, 0.25, 0.0], [0.25, 1.0, 0.0], [0.0, 0.0, 1.0], [0.0625, 0.25, 0.0]]

So the follow up commits fix the problems and update the expected values. Here is a script that I used to the bbox methods operate as expected by comparing them to the equivalent loss methods:

from functools import partial

import torch
from torchvision import ops


def loss_to_iou(a, b, fn):
    # Convert Loss to IoU
    return 1.0 - fn(a, b)


def gen_box(size):
    # Build proper bboxes
    xy1 = torch.rand((size, 2))
    xy2 = xy1 + torch.rand((size, 2))
    return torch.cat([xy1, xy2], axis=-1)


def cartesian_product_slow(boxes1, boxes2, fn):
    # Extemely slow method for testing cartesian product and use of the loss fn
    N = boxes1.size(0)
    M = boxes2.size(0)

    result = torch.zeros((N, M))
    for i in range(N):
        for j in range(M):
            result[i, j] = fn(boxes1[i].unsqueeze(0), boxes2[j].unsqueeze(0))

    return result


def cartesian_product_fast(boxes1, boxes2, fn):
    # Faster method for testing cartesian product and use of the loss fn
    N = boxes1.size(0)
    M = boxes2.size(0)

    bx_l = boxes1.repeat_interleave(M, dim=0)
    bx_r = boxes2.repeat((N, 1))

    result = fn(bx_l, bx_r)

    return result.view(N, M)


N = 5
M = 7

boxes1 = gen_box(N)
boxes2 = gen_box(M)

# Check that the slow and fast are equivalent
a = cartesian_product_fast(boxes1, boxes2, partial(loss_to_iou, fn=ops.distance_box_iou_loss))
b = cartesian_product_slow(boxes1, boxes2, partial(loss_to_iou, fn=ops.distance_box_iou_loss))
assert torch.allclose(a, b)

# Check the build-in Generalized IoU method against the cartesian of the equivalent loss
a = cartesian_product_fast(boxes1, boxes2, partial(loss_to_iou, fn=ops.generalized_box_iou_loss))
b = ops.generalized_box_iou(boxes1, boxes2)
assert torch.allclose(a, b)

# Check the build-in Distance IoU method against the cartesian of the equivalent loss
a = cartesian_product_fast(boxes1, boxes2, partial(loss_to_iou, fn=ops.distance_box_iou_loss))
b = ops.distance_box_iou(boxes1, boxes2)
assert torch.allclose(a, b)

# Check the build-in Complete IoU method against the cartesian of the equivalent loss
a = cartesian_product_fast(boxes1, boxes2, partial(loss_to_iou, fn=ops.complete_box_iou_loss))
b = ops.complete_box_iou(boxes1, boxes2)
assert torch.allclose(a, b)

print("OK")

datumbox and others added 2 commits August 1, 2022 19:58
Co-authored-by: Abhijit Deo <72816663+abhi-glitchhg@users.noreply.github.com>
datumbox added a commit to datumbox/vision that referenced this pull request Aug 2, 2022
@datumbox datumbox merged commit ea0be26 into pytorch:main Aug 2, 2022
@@ -325,13 +325,13 @@ def complete_box_iou(boxes1: Tensor, boxes2: Tensor, eps: float = 1e-7) -> Tenso

diou, iou = _box_diou_iou(boxes1, boxes2, eps)

w_pred = boxes1[:, 2] - boxes1[:, 0]
h_pred = boxes1[:, 3] - boxes1[:, 1]
w_pred = boxes1[:, None, 2] - boxes1[:, None, 0]
Copy link
Contributor

Choose a reason for hiding this comment

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

why this is needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is necessary for the broadcasting to work properly. See also:

lti = torch.min(boxes1[:, None, :2], boxes2[:, :2])
rbi = torch.max(boxes1[:, None, 2:], boxes2[:, 2:])
whi = _upcast(rbi - lti).clamp(min=0) # [N,M,2]
diagonal_distance_squared = (whi[:, :, 0] ** 2) + (whi[:, :, 1] ** 2) + eps

datumbox added a commit that referenced this pull request Aug 2, 2022
* Fix d/c IoU for different batch sizes (#6338)

* Fix tests

* Fix linter
@oke-aditya oke-aditya deleted the fix_g/c_iou branch August 2, 2022 10:48
facebook-github-bot pushed a commit that referenced this pull request Aug 2, 2022
Summary:
* Fix bug in calculating cIoU for unequal sizes

* Remove comment

* what the epsilon?

* Fixing DIoU

* Optimization by Francisco.

* Fix the expected values on CompleteBoxIoU

* Apply suggestions from code review

* Adding cartesian product test.

* remove static

Reviewed By: NicolasHug

Differential Revision: D38351751

fbshipit-source-id: 097e5f7048c650767e275fbb2c30ed0c800b1314

Co-authored-by: Abhijit Deo <72816663+abhi-glitchhg@users.noreply.github.com>
Co-authored-by: Vasilis Vryniotis <vvryniotis@fb.com>
Co-authored-by: Vasilis Vryniotis <datumbox@users.noreply.github.com>
Co-authored-by: Abhijit Deo <72816663+abhi-glitchhg@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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