Add boxes V2 as discussed in #1142 (superseed #1177)

[hal-314]

Changes

This PR implements #1142 (issue) and superseeds #1177 (pr). It's the same code with the following differences:

• It takes the object class approach discussed in A couple of questions about bbox module functions #1142.
• It creates two new classes: Boxes and Boxes3D. They are torchscript classes to allow to be used in traced / scripted code. Without using torch.jit.script decorator, I faced several strange behaviors when scripting.
• Fixes gradient check bugs

Check #1177 for differences with current kornia.geometry.bbox module.

#### TODO
- Update docs
- As suggested by shijianjian in #1177, better explanation and name for *_plus_1 in to_tensor and from_tensor.
~~- Fix gradient check test fail for Boxes3D.to_tensor. I don't understand why it's happening as it's almost an exact copy of Boxes.to_tensor ~~
- ¿Update CHANGELOG? Is this something I should do?

TODO to do in future PRs):

• Port kornia containers to use them. At least, expose bbox_v2.
  - Check transforms. Care should be taken since transforms assumes +1 convention. At least, it happens with random flips. Check this article. See Clarify keypoints behaviour and their limitations #1398. As temporary fix, we use "+1" convention internally. See whole temporary fix is done in commit "07795f9c78468b42ecd96f4d5d2b8df562deecf7".

Type of change

• 🔬 New feature (non-breaking change which adds functionality)

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• Did you update CHANGELOG in case of a major change?

[shijianjian]

LGTM.

[edgarriba]

don't forget to add to docs

[edgarriba]

@hal-314 @shijianjian can you consider this here ? #1194 (comment)

[hal-314]

@edgarriba now, matrix transform must be (3,3) when user created N boxes (N, 4, 2) or (B, 3,3) when user pass a batch of N boxes (B,N,4,2). So, it would work fine with the new api:

boxes = Boxes.from_tensor(torch.randn(10, 4))
boxes.transform_boxes(torch.eye(3))

Also, the other reported bug regarding coordinates position is fixed when converting back to xywh: boxes.to_tensor(mode='xywh')

[hal-314]

Hi again @edgarriba
I added docs. There only failing 1 test gradient check test for 3D boxes. I don't know how to fix it. The code it's the same as Boxes :|
Finally, from PR #1177, I don't find a better name for mode values xyxy_plus_1, etc in from_tensor and to_tensor. Any suggestions?

[edgarriba]

Hi again @edgarriba I added docs. There only failing 1 test gradient check test for 3D boxes. I don't know how to fix it. The code it's the same as Boxes :| Finally, from PR #1177, I don't find a better name for mode values xyxy_plus_1, etc in from_tensor and to_tensor. Any suggestions?

maybe just xyxy_plus ?

[hal-314]

@edgarriba The remaining points in this PR are:

1. Fix gradient checks tests for Boxes3D -> I need help as I don't know how to solve it. See above.
2. DeepSource errors regarding in test. They should be use staticmethod. ¿How can I disable it? Other kornia tests doesn't raise this error when they are exactly the same.

[shijianjian]

1. Fix gradient checks tests for Boxes3D -> I need help as I don't know how to solve it. See above.

I will take a look on it tomorrow. It is also possible that TV has a bad support for 3D ops and tensors.

2. DeepSource errors regarding in test. They should be use staticmethod. ¿How can I disable it? Other kornia tests doesn't raise this error when they are exactly the same.

No worries on that. They are merely some coding pattern recommendations. I did not pretty follow that either.

[shijianjian]

@hal-314 It looks like there are many numeric errors in 3D boxes. Can you take a look?

[edgarriba]

Marked as draft since tests seems to be broken

[hal-314]

Fix bugs related to "xyxy_plus" and "vertices_plus" + renaming from "bbox_v2" to "boxes" + implemented workaround discussed in #1398 . Now, I think it's ready to merge.

Note that the whole #1398 workaround is implemented in one unique commit (07795f9) so it can be easily reverted in the future.

Finally, there are two tasks left regarding the new boxes approach:

1. Make containers to use the new api. <-- I can work on it.
2. Deprecate kornia.geometry.bbox.py . <-- I don't know how do you want to handle it.
   I would suggest to tackle them in two separated PRs.

Marked as draft since tests seems to be broken

Those failing tests weren't related to this PRs. I updated the branch with the last commit in the master in hope they get fixed. So, I remove the draft labeling.

[shijianjian]

One thing of concern is that if Boxes module cannot be registered as an nn.Module, then it cannot be chained. Say,

class MyBoxProcessor(nn.Module):
    def __init__(self,):
        self.box = Boxes(...)

MyBoxProcessor().to("cuda")

Then the box variable will still be in CPU. Probably put it to the backlog for now. We are still unclear about how this Boxes module will be used in the future. Let's udpate it step by step.

[hal-314]

One thing of concern is that if Boxes module cannot be registered as an nn.Module, then it cannot be chained. Say,

class MyBoxProcessor(nn.Module):
    def __init__(self,):
        self.box = Boxes(...)

MyBoxProcessor().to("cuda")

Then the box variable will still be in CPU. Probably put it to the backlog for now. We are still unclear about how this Boxes module will be used in the future. Let's udpate it step by step.

As workaround, you could override to, cpu, cuda, etc calls like below to get the same behavior as extending as a standard nn.Module. It isn't the perfect solution but it should cover many cases.

class MyBoxProcessor(nn.Module):
    def __init__(self,):
        super().__init__()
#         self.box = Boxes(...)
        
    def to(self, *args, **kwargs):
        print('hi')
        # self.box = self.box.to(*args, **kwargs)
        super().to(*args, **kwargs)

MyBoxProcessor().to('cuda')

[shijianjian]

@hal-314 Yes. We have to move to nn.Module if the situation above is the most common cases. Let's find out later.

[hal-314]

In previous commits, tests were failing in test_boxes.py. the error wasn't related to my code. It was something related to "_IO.string" (I don't access to the CI logs of previous commits). However, boxes tests runs successfully in my local machine: pytest -v --device all --dtype float32 --cov=kornia test/geometry/test_boxes.py --flake8 --mypy. I can run any more the whole kornia tests as they freeze my laptop (8gb ram).

If it happens again, I don't know why and how I could fix them :/

[shijianjian]

Hi @hal-314, can you fix the doctest error so that we can merge?

[edgarriba]

LGTM. @shijianjian merge when green lights

updated

[shijianjian]

Consider this in the future: https://github.com/lilanxiao/Rotated_IoU