Disable integer dtype for rotated bounding boxes#9133
Disable integer dtype for rotated bounding boxes#9133NicolasHug merged 14 commits intopytorch:mainfrom
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/9133
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (2 Unrelated Failures)As of commit 6fd0d51 with merge base 5d6e039 ( BROKEN TRUNK - The following jobs failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
There was a problem hiding this comment.
Thanks @AntoineSimoulin ! Made non-blocking comments below, approving assuming the CI is happy
| requires_grad: bool | None = None, | ||
| ) -> BoundingBoxes: | ||
| tensor = cls._to_tensor(data, dtype=dtype, device=device, requires_grad=requires_grad) | ||
| cls._check_format(tensor, format=format) |
There was a problem hiding this comment.
Since we call it only once and it's only 2 lines, we can inline it instead of making it a method. Also it might be best to do the validation before as the very first step, before calling cls._to_tensor()?
There was a problem hiding this comment.
@NicolasHug, I included it within the function. However, it possible to pass list as input so it easier to run this test after the input has been converted to a tensor.
| if clamping_mode == "hard": | ||
| bounding_boxes[..., 0].clamp_(0) # Clamp x1 to 0 |
There was a problem hiding this comment.
It's not immediately obvious that this relates to dtypes, so just flagging to make sure this change is intended?
There was a problem hiding this comment.
@NicolasHug good catch. Yeah this was related to dtype and introduction of epsilon. I have remove it in a later commit attached to this PR.
|
Forgot to add: before merging, let's add a small test in |
|
Added a small test in test_tv_tensors.py ensuring the error is raised on the rotated formats pytest test/test_tv_tensors.py -v -k "test_bbox_format_dtype" |
| # TODO there is a 1e-6 difference between GPU and CPU outputs | ||
| # due to clamping. To avoid failing this test, we do clamp before hand. |
There was a problem hiding this comment.
Thanks for writing this TODO, for GPU vs CPU it's typically OK to have differences of up to 1e-4. We should be able to pass atol and rtol to the check_kernel call, but we can address that later
|
Hey @NicolasHug! You merged this PR, but no labels were added. |
Co-authored-by: Nicolas Hug <contact@nicolas-hug.com> Co-authored-by: Nicolas Hug <nh.nicolas.hug@gmail.com>
Reviewed By: AntoineSimoulin Differential Revision: D79175049 fbshipit-source-id: 28e287b1dd7d874f060c220014b978a00803ba90 Co-authored-by: Nicolas Hug <contact@nicolas-hug.com> Co-authored-by: Nicolas Hug <nh.nicolas.hug@gmail.com>
Context
This PR disables integer
dtypefor rotated bounding boxes. Indeed rotated bounding boxes typically have floating coordinates for vertices. Functions to generate boxes or to transform boxes will typically involves trigonometric sinus and cosinus functions, which does not guarantees the coordinates will be integer. Rounding to the nearest integer can lead to degenerated boxes. Chaining transformation can further exacerbate the degeneration as rounding error will compound. For these reason, we choose to raise an error in theBoundingBoxesconstructor if a integerdtypetensor is passed together with a rotated bounding box format. This check is done in the_check_formatstatic function.Testing
We adapt the tests with the following condition to verify that tests combining integer dtype and rotated bounding box formats are indeed failing.
We finally remove artifacts in the testing and transforms added to make sure the transforms were compatible with integer
dtypefor rotated boxes. This includes all the rounding operations as well as epsilon offsets in the testing.Please run tests with:
pytest test/test_transforms_v2.py -k box -v ... 3230 passed, 5686 deselected, 678 xfailed in 129.91s (0:02:09)