-
Notifications
You must be signed in to change notification settings - Fork 7.1k
Disable integer dtype for rotated bounding boxes #9133
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @AntoineSimoulin ! Made non-blocking comments below, approving assuming the CI is happy
@@ -111,6 +116,7 @@ def __new__( | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
dtype
for 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 theBoundingBoxes
constructor if a integerdtype
tensor is passed together with a rotated bounding box format. This check is done in the_check_format
static 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
dtype
for 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)