Skip to content

Conversation

AntoineSimoulin
Copy link
Member

@AntoineSimoulin AntoineSimoulin commented Jun 30, 2025

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 the BoundingBoxes constructor if a integer dtype 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.

if not dtype.is_floating_point and (
            tv_tensors.is_rotated_bounding_format(old_format) or tv_tensors.is_rotated_bounding_format(new_format)
        ):
            pytest.xfail("Rotated bounding boxes should be floating point tensors")

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)

Copy link

pytorch-bot bot commented Jun 30, 2025

🔗 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 (image):

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.

Copy link
Member

@NicolasHug NicolasHug left a 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)
Copy link
Member

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()?

Copy link
Member Author

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.

Comment on lines 577 to 578
if clamping_mode == "hard":
bounding_boxes[..., 0].clamp_(0) # Clamp x1 to 0
Copy link
Member

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?

Copy link
Member Author

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.

@NicolasHug
Copy link
Member

Forgot to add: before merging, let's add a small test in test_tv_tensors.py ensuring the error is raised on the rotated formats

@AntoineSimoulin
Copy link
Member Author

AntoineSimoulin commented Jul 1, 2025

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"

Comment on lines +2024 to +2025
# 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.
Copy link
Member

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

@NicolasHug NicolasHug mentioned this pull request Jul 1, 2025
@NicolasHug NicolasHug merged commit fb3926e into pytorch:main Jul 1, 2025
57 of 61 checks passed
Copy link

github-actions bot commented Jul 1, 2025

Hey @NicolasHug!

You merged this PR, but no labels were added.
The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py

AntoineSimoulin added a commit to AntoineSimoulin/vision that referenced this pull request Jul 1, 2025
Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>
Co-authored-by: Nicolas Hug <nh.nicolas.hug@gmail.com>
facebook-github-bot pushed a commit that referenced this pull request Jul 31, 2025
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants