Intersection over Union Metric/Loss

[briankosw]

What does this PR do?

Implements IoU metric and IoU loss

Fixes #370. Related to #347 and #251.

Before submitting

• Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure your PR does only one thing, instead of bundling different changes together?
• Did you make sure to update the documentation with your changes?
• Did you write any new necessary tests? [not needed for typos/docs]
• Did you verify new and existing tests pass locally with your changes?
• If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

• Is this pull request ready for review? (if not, please submit in draft mode)

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

[codecov]

Codecov Report

Merging #469 (17a3262) into master (a72766c) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #469      +/-   ##
==========================================
+ Coverage   80.52%   80.57%   +0.04%     
==========================================
  Files         103      103              
  Lines        5710     5724      +14     
==========================================
+ Hits         4598     4612      +14     
  Misses       1112     1112              

Flag	Coverage Δ
cpu	25.54% <100.00%> (+0.18%)	⬆️
pytest	25.54% <100.00%> (+0.18%)	⬆️
unittests	80.13% <100.00%> (+0.04%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pl_bolts/losses/object_detection.py	100.00% <100.00%> (ø)
pl_bolts/metrics/object_detection.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a72766c...17a3262. Read the comment docs.

[oke-aditya]

But IoU and gIoU are available in torchvision. Thoughts?
I agree that gIoU loss IoU loss is not available in torchvision (hopefully soon) but for ops, there is tested support.

cc @akihironitta maybe we are reimplementing these in bolts to avoid dependency?

[briankosw]

But IoU and gIoU are available in torchvision. Thoughts ?
I agree that gIoU loss IoU loss is not available in torchvision (hopefully soon) but for ops, there is tested support.

cc @akihironitta maybe we are reimplementing these in bolts to avoid dependency ?

The conversation regarding pl_bolts having its own implementation of giou and giou_loss was held in #347. The conclusion was that it's helpful to have a separate version, hence this PR.

[oke-aditya]

Hmm, not an issue then but we would need to test to match torchvision's implementation exactly.
E.g. IoU and gIoU in torchvision return a Tensor[N, 4] while here we do a little bit different.
Which might be slightly confusing for users.

It can be nice to have drop-in replacement for consistency. Thoughts ?

cc @akihironitta @Borda

[briankosw]

Hmm, not an issue then but we would need to test to match torchvision's implementation exactly.
E.g. IoU and gIoU in torchvision return a Tensor[N, 4] while here we do a little bit different.
Which might be slightly confusing for users.

So IoU and GIoU in torchvision actually doesn't return an Nx4 tensor, it returns an NxM tensor, where N is the dimension of the first set of boxes and M is the dimension of the second set of boxes. In fact my implementation does the same, and you bring up an excellent point that perhaps the docstring should correctly reflect this fact to the users. In addition, I can further demonstrate this by creating tests that reflect this fact. Does that sound ok?

[oke-aditya]

Oh yes, it returns Tensor[N, M] Yes, we can even test on same values as we did in torchvision.
Tests in torchvision are here

It would nice to demonstrate with docstring as well. Maybe just say

Returns:
        iou (Tensor[N, M]): the NxM matrix containing the pairwise IoU values for every element in boxes1 and boxes2

Like in torchvision

[briankosw]

Great suggestion @oke-aditya! How should I go about changing the description for gIoU? Do I create a separate PR?

[oke-aditya]

I'm Not sure about that. cc @akihironitta 😄

[akihironitta]

But IoU and gIoU are available in torchvision. Thoughts?
I agree that gIoU loss IoU loss is not available in torchvision (hopefully soon) but for ops, there is tested support.

cc @akihironitta maybe we are reimplementing these in bolts to avoid dependency?

Since Bolts already (heavily?) depends on torchvision, I think we can import torchvision's (g)iou and use them here. It's also nice that they should be tested well by the torchvision team. In #347, @briankosw asked if we should have own implementation, but it seems (and I'm sorry) that we missed the question. So, for now, let's hear what others think of this. cc: @PyTorchLightning/bolts-contributors @Borda

Is there any difference between this and this one from torchvision except that we extract giou from 1 in this?

Ah I didn't realize torchvision has its own implementation. You're correct in that we subtract giou from 1. Should I change it to use torchvision's giou?

---

It would nice to demonstrate with docstring as well. Maybe just say

@briankosw Let's improve the description of the loss in the docs. Could you work on this in another PR?

[briankosw]

I'll pause work on this PR then. I already updated the docs for IoU metric as @oke-aditya suggested, so what description should I improve? @akihironitta

[akihironitta]

@briankosw I left a suggestion. Would you mind considering it?

[akihironitta]

I'll pause work on this PR then.

Right, let's wait for what other members have to say about #469 (comment).

[oke-aditya]

Also a small point, If we had to add say CIoU and DIoU (yeah IoUs are so many)
We could potentially then have a combined function.

def iou(type="ciou", boxes1: Tensor, boxes2: Tensor):

[Borda]

@akihironitta mind re-review? 🐰

[akihironitta]

LGTM!