draw_bounding_boxes should check that (W, H) >= (xmax, ymax) >= (xmin, ymin) >= (0, 0)

[norabelrose]

🚀 The feature

Currently, draw_bounding_boxes does not sanity check the input bounding boxes to ensure that (xmax, ymax) >= (xmin, ymin). This can lead to unexpected output that can be difficult to debug if the user, say, inputs boxes in cxcywh format.

I could make a PR for this if needed but would prefer if someone else did it since I have my hands full with a PR I'm working on for PyTorch proper.

Motivation, pitch

I personally just ran into this problem and I thought something was wrong with my network for a while.

Currently, the box conversion ops do this check, and it seems at least as important to do the same check in draw_bounding_boxes.

Alternatives

No response

Additional context

No response

[pmeier]

I agree with the overall sentiment here. My question is how you got to the situation. Did you know that you had "cxcywh" bounding boxes and missed that draw_bounding_boxes requires "xyxy" or did you not know the format before and only started to look into it after you got wrong results?

In the former case, we also could have a format="xyxy" parameter on the function, since we know how to convert bounding boxes. In general though, a smoke test after the conversion is still something we could do.

We are currently in the process of introducing new abstractions for bounding boxes. With that, bounding boxes store information about the format and thus a conversion as proposed above would be handled automatically.

vision/torchvision/prototype/features/_image.py

Lines 83 to 86 in 71d2bb0

	def draw_bounding_box(self, bounding_box: BoundingBox, **kwargs: Any) -> Image:
	# TODO: this is useful for developing and debugging but we should remove or at least revisit this before we
	# promote this out of the prototype state
	return Image.new_like(self, draw_bounding_boxes(self, bounding_box.to_format("xyxy").view(-1, 4), **kwargs))

In an internal discussion following #5500 (comment), we decided to move this functionality into torchvision.utils just before we migrate the new abstractions to the main area.

[norabelrose]

@pmeier I knew that draw_bounding_boxes requires "xyxy", but I got confused about which format my bounding boxes were at that particular point in the code, since I'm using a DETR model where the model outputs cxcywh, but xyxy is used for the GIOU loss, and my dataset uses xyxy format.

The class abstraction definitely sounds like a good idea. I still think though that it would be a good idea to add this check to the "functional" version draw_bounding_box API, especially since the conversion ops already do it.

[NicolasHug]

@norabelrose thanks for the suggestion, we'd be happy to review a PR that adds these checks

[oke-aditya]

To avoid code duplication and for convenience.

I suggest we create a small util _check_box_fmt. We need these is multiple places. box_iou, generalized_box_iou, nms and draw_bounding_boxes.

With this single util we could check all places and raise ValueError suggesting to either check the input or use box_convert to change the format.

cc @NicolasHug @norabelrose
Let me know if you didn't have bandwidth / need help.

[NicolasHug]

There was some discussions before about adding these checks in the ops (box_iou, nms etc.). We don't do it because this would require adding a synchronization point of the CUDA streams. It's probably best to ignore the ops for now and stick to draw_bounding_boxes

[singhularAdi]

I could look into this if no one else is.

[pmeier]

Sure, go ahead!

[oke-aditya]

Go ahead @singhularAdi Feel free to ping Nicolas for review :)
Note that you should throw a ValueError if the above case occurs

Edit:
Wait a sec. I think I know you. You are my college senior. 😄 World is such a nice circle.