Allow users to choose the bbox clamping mode #9128
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. 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.
@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.
|
|
||
| # 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.
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.
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.
Should we consider having possible values be either "hard", "soft" and None (no value provided) instead of having the "none" value as a string?
There was a problem hiding this comment.
I think that would be a good option, we'll have to make sure this doesn't conflict with the None default parameter of ClampBoundingBoxes and its kernel. Let's consider this soon in a follow-up
As long as the box itself must have a value (and I think that's the case), then I think that makes sense. |
scotts
left a comment
There was a problem hiding this comment.
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.
|
Hey @NicolasHug! You merged this PR, but no labels were added. |
Reviewed By: AntoineSimoulin Differential Revision: D79175015 fbshipit-source-id: 078ad9a1250cc478685cb890e4d6ac636da835a4
Closes #8254
This PR:
clamping_modeparameter 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_modeparameter to all necessary kernels.clamping_modeparameter to theClampBoundingBoxes()method. The default here isNone, which defaults to the bbox'sclamping_modeattribute. @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_modeattribute of a bboc and can safely be used within aCompose()pipeline.I left a bunch of non-critical
TODOBBtodos 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 :)cc @vfdev-5