Added CIOU loss function

[abhi-glitchhg]

#2980

Reference

cc: @oke-aditya @datumbox

[oke-aditya]

Hi. It's late in India rn. Will review it in daytime.

[datumbox]

@abhi-glitchhg Thanks for the contribution!

I've added a few comments for your attention.

[oke-aditya]

Nice a few comments though

[yassineAlouini]

Working on dIoU, I have noticed that many things can be refactored between gIoU, cIoU, and dIoU.
I will push my contribution anyway but we can discuss what can be done next to improve things. 👌

[oke-aditya]

yeah :) There is a neat relation between these 3 and code can be re-used. 😄

[yassineAlouini]

yeah :) There is a neat relation between these 3 and code can be re-used. smile

Yes, I guess we can repeat some of the code for now since we have two branches at the same time + the existing gIoU. Next, we can start a new PR to refactor things a bit.

[datumbox]

@abhi-glitchhg @oke-aditya Thank you very much for your contributions and reviews so far.

Apologies for the delayed response, but I was OOO. I might be missing something but what is the reason we have duplicate implementations for cIoU in the loss and the box version? Is there a reason we don't share some of the code?

[yassineAlouini]

@abhi-glitchhg @oke-aditya Thank you very much for your contributions and reviews so far.

Apologies for the delayed response, but I was OOO. I might be missing something but what is the reason we have duplicate implementations for cIoU in the loss and the box version? Is there a reason we don't share some of the code?

As far as my exploration goes, it seems it is the case for gIoU as well and I haven't found any reason why this should be the case but maybe I am missing something. I have also duplicated some code in dIoU. Perhaps it is best to have a PR to refactor some of the shared code between gIoU, cIoU, and dIoU?

[datumbox]

@yassineAlouini I think sending a separate PR to refactor gIoU makes a lot of sense. For new code (cIoU and dIoU) I would fix in the current open PRs.

[yassineAlouini]

@datumbox Noted, I will work on this (when I have allocated time) and update the code. 👌

[datumbox]

@abhi-glitchhg Just a minor comment below. When you are ready for us to check the PR, remove the draft flag. :)

[datumbox]

@abhi-glitchhg Thanks for the contribution.

I'm looping @yassineAlouini who is looking at dIoU as many of the comments below are applicable.

I've verified the implementation both by reviewing and by comparing the two methods as follows:

import torch

from torchvision.ops.boxes import complete_box_iou
from torchvision.ops.ciou_loss import complete_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 - complete_box_iou(box1, box2)
    v2 = complete_box_iou_loss(box1, box2)

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

This confirms that both implementations are equivalent. Though I think the PR is correct, I feel there is need for deep cleaning both for dIoU and cIoU (and gIoU). In particular we should consider refactoring the code across losses and boxes to share some of the internal implementations. Not everything is shareable and we should be mindful to maintain the speed but we shouldn't have duplicate implementations. Moreover, @pmeier suggested a few improvements on the test side at #5786 (comment), so these improvements should also be completed.

Kindly let me know what direction you would like us to take now. We could bring both PRs on a similar state as this one and merge and then 1 of you can pick up the cleaning bit. Alternatively we can keep iterating on the current PRs until the nits are taken care of. It's up to you and I'm happy to take your preferred approach, I just want to make sure the implementations will be cleaned up prior the next release.

[datumbox]

Nit:
We got methods in ops.boxes for estimating intersection and unions. I don't think we can just use them here as-is, but it's worth considering refactoring the entire loss area to avoid re-estimating quantities and instead try use some of the methods from boxes.

Moreover it's worth noting that cIoU and dIoU share a large number of common code that could be shared.

[abhi-glitchhg]

Kindly let me know what direction you would like us to take now. We could bring both PRs on a similar state as this one and merge and then 1 of you can pick up the cleaning bit. Alternatively we can keep iterating on the current PRs until the nits are taken care of. It's up to you and I'm happy to take your preferred approach, I just want to make sure the implementations will be cleaned up prior the next release.

I would prefer the first approach(bring both PRs to similar state, merge, and then do refactor) but I'm also open to the second approach.

[datumbox]

@abhi-glitchhg Sounds good to me. Approving to unblock. We should do the clean ups once dIoU is merged.

@pmeier Any concerns/blocking changes on your side for tests?

[pmeier]

Minor style nit, but otherwise testing looks good to me minus the things we discussed for the follow up PR.

[abhi-glitchhg]

Hey, @datumbox , what's the expected date for next release?

[pmeier]

Tentative date for branch cut, i.e. "feature freeze", is on 19th of May. After that it will be roughly one month until release, but no official date is set yet.