Conversation
| return BoundingBox( | ||
| x1=max(0, min(self.x1, max_width)), | ||
| y1=max(0, min(self.y1, max_height)), | ||
| x2=max(0, min(self.x2, max_width)), | ||
| y2=max(0, min(self.y2, max_height)), | ||
| ) | ||
|
|
There was a problem hiding this comment.
The clamp method might produce a bounding box with invalid dimensions (e.g., x1 == x2 or y1 == y2) when the original coordinates are out of bounds. Consider adjusting the logic or adding a safeguard to ensure the clamped bounding box remains valid.
| return BoundingBox( | |
| x1=max(0, min(self.x1, max_width)), | |
| y1=max(0, min(self.y1, max_height)), | |
| x2=max(0, min(self.x2, max_width)), | |
| y2=max(0, min(self.y2, max_height)), | |
| ) | |
| x1 = max(0, min(self.x1, max_width)) | |
| y1 = max(0, min(self.y1, max_height)) | |
| x2 = max(0, min(self.x2, max_width)) | |
| y2 = max(0, min(self.y2, max_height)) | |
| # Ensure the clamped bounding box has valid dimensions | |
| if x1 >= x2: | |
| x2 = x1 + 1 | |
| if y1 >= y2: | |
| y2 = y1 + 1 | |
| return BoundingBox(x1, y1, x2, y2) |
There was a problem hiding this comment.
The clamp method is meant only to restrict the coordinates to the valid range, not to fix or validate the structure of the bounding box itself.
There was a problem hiding this comment.
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
open_image_models/detection/core/base.py:83
- The iter method returns the bounding box coordinates as (x1, y1, x2, y2), which differs from the to_xywh method that returns (x1, y1, width, height). Consider clarifying this difference in the docstring to avoid confusion.
def __iter__(self) -> Iterator[int]:
open_image_models/detection/core/base.py:133
- The new from_detection_data method for DetectionResult is not covered by tests in this PR. Consider adding tests to verify that DetectionResult instances are correctly created from bounding box data, confidence scores, and class labels.
@classmethod
| :return: A new, clamped `BoundingBox`. | ||
| """ |
There was a problem hiding this comment.
The clamp method directly clamps each coordinate without validating the original bounding box order. In cases where the input box is already invalid (for example, x1 >= x2), the clamping may produce an inverted or degenerate box. Consider handling or documenting the behavior for such inputs.
| :return: A new, clamped `BoundingBox`. | |
| """ | |
| :return: A new, clamped `BoundingBox`. | |
| :raises ValueError: If the bounding box is invalid. | |
| """ | |
| if not self.is_valid(max_width, max_height): | |
| raise ValueError("Cannot clamp an invalid bounding box.") |
from_detection_datautilityclamp,is_validand__iter__methods to BoundingBox