-
Notifications
You must be signed in to change notification settings - Fork 7.1k
Allow users to choose the bbox clamping mode #9128
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
base: main
Are you sure you want to change the base?
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/9128
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ❌ 1 New FailureAs of commit faffedf with merge base 6aee5ed ( NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
def transform(self, inpt: tv_tensors.BoundingBoxes, params: dict[str, Any]) -> tv_tensors.BoundingBoxes: | ||
out = inpt.clone() | ||
out.clamping_mode = self.clamping_mode | ||
return out |
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.
@AntoineSimoulin @scotts in our chat I raised a potential problem with the clamping_mode
leaking out of this transform. I think I made our conversation a lot more complicated that it needed to be. We can avoid the "leak" by just calling clone(), as we should, which returns a copy.
No need to make Compose()
work like a context manager. I added corresponding tests.
@@ -46,6 +46,14 @@ def is_rotated_bounding_format(format: BoundingBoxFormat) -> bool: | |||
) | |||
|
|||
|
|||
# TODOBB consider making this a Literal instead. Tried briefly and got | |||
# torchscript errors, leaving to str for now. | |||
# CLAMPING_MODE_TYPE = Literal["hard", "soft", "none"] |
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.
Regarding the acceptable values, I'm not a fan of "none"
, it might be confused with None
. Maybe "no-clamping"
is better? Regardless, if that's OK with you I think we can leave this (nonetheless important!) bikeshedding for later PR.
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.
Agreed that None
and "none"
are bound (ha!) to get confused and that we should definitely choose something else.
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.
Should we consider having possible values be either "hard", "soft" and None (no value provided) instead of having the "none" value as a string?
As long as the box itself must have a value (and I think that's the case), then I think that makes sense. |
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.
I think it's a testament to the transform's design that this was so easy to add! This also makes me even more comfortable we made the right design decision, since we don't need to touch the individual transforms.
This PR:
clamping_mode
parameter to to theBoundingBoxes()
constructor which is stored as a metadata attribute. The default is "hard" for now because this is what happens inmain
. We will change it to "soft" as soon as Adjust clamping for rotated bboxes #9112 is merged.clamping_mode
parameter to all necessary kernels.clamping_mode
parameter to theClampBoundingBoxes()
method. The default here isNone
, which defaults to the bbox'sclamping_mode
attribute. @AntoineSimoulin @scotts that's a different default from what we had originally discussed, but I think it makes sense?SetClampingMode()
transform which just sets theclamping_mode
attribute of a bboc and can safely be used within aCompose()
pipeline.I left a bunch of non-critical
TODOBB
todos in the code, which I'll address later, but before the release. The type checker is failing, I'll address before merging, please review regardless :)