Simplify query_bounding_boxes logic #7786
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/7786
Note: Links to docs will display an error until the docs builds have been completed. ❌ 2 New Failures, 2 PendingAs of commit b962a4b: NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
| if tensor.ndim == 1: | ||
| tensor = tensor.unsqueeze(0) | ||
| elif tensor.ndim != 2: | ||
| raise ValueError(f"Expected a 1D or 2D tensor, got {tensor.ndim}D") |
There was a problem hiding this comment.
I don't have a super strong opinion on that and I don't mind removing it, but IMO passing other number of dims indicates a user mistake upstream.
We don't support batches of bounding boxes (i.e. we can't batch BoundingBoxes objects where num_bboxes differs per object) so I'm not sure we should pretend that we do.
There was a problem hiding this comment.
Due to this, the following check is obsolete:
vision/torchvision/transforms/v2/_misc.py
Lines 392 to 393 in 9b82df4
I've removed it and the (now failing) test for it in b962a4b. If we decide to not go with this, make sure to revert the commit as well.
test/common_utils.py
Outdated
| @@ -638,6 +638,7 @@ def make_bounding_box( | |||
| dtype=None, | |||
| device="cpu", | |||
| ): | |||
| batch_dims = () # This is nasty but this whole thing will be removed soon. | |||
There was a problem hiding this comment.
@pmeier I had to add this to fix the tests. I'm sure there's a better way to fix it, but I don't want to spend time on this (already spent 10+ minutes) as these tests an helpers will be removed.
Feel free to push a better fix if you prefer.
There was a problem hiding this comment.
as these tests an helpers will be removed.
You put the hack into make_bounding_box, which will not be removed. I've moved it into the helpers that will actually be removed. Nothing new is failing after that. Meaning, we are not misusing make_bounding_box anywhere from the new side of the tests.
|
@NicolasHug the errors you were seeing were all multiple manifestations of the same issue: some bounding box references did not correctly stack after applying the reference only to a single transform. I've fixed that in d360904 and tests should pass now. |
| @@ -691,7 +691,7 @@ def make_bounding_box_loader(*, extra_dims=(), format, spatial_size=DEFAULT_PORT | |||
| if isinstance(format, str): | |||
| format = datapoints.BoundingBoxFormat[format] | |||
|
|
|||
| spatial_size = _parse_size(spatial_size, name="canvas_size") | |||
| spatial_size = _parse_size(spatial_size, name="spatial_size") | |||
|
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 |
Summary: Co-authored-by: Philip Meier <github.pmeier@posteo.de> Reviewed By: matteobettini Differential Revision: D48642261 fbshipit-source-id: c0af539c083d3d00289ff4e39901d18887169b63
We don't hard-enforce one single bbox instance per sample in
query_bounding_boxes: it's not the place to enforce such a thing, even though ideally we would indeed enforce it (but how/where to do that without perf hit is unclear).I also documented this assumption which was previously undocumented anywhere as far as I know. We'll also have to hammer it down when we re-write the docs / tutorials.
cc @vfdev-5