-
Notifications
You must be signed in to change notification settings - Fork 7.2k
Add clamping_mode
for KeyPoints
#9234
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/9234
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 37d73d4 with merge base 58eb039 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
clamping_mode
for KeyPointsclamping_mode
for KeyPoints
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.
Looks good to me overall. Just a few minor remarks.
clamping_mode: Default is "auto" which relies on the input keypoint' | ||
``clamping_mode`` attribute. | ||
The clamping is done according to the keypoints' ``canvas_size`` meta-data. | ||
Read more in :ref:`clamping_mode_tuto` |
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.
clamping_mode_tuto
in the docs currently only covers bounding boxes and would need to be updated as well.
import torch | ||
|
||
from ._bounding_boxes import BoundingBoxes, BoundingBoxFormat, is_rotated_bounding_format | ||
from ._bounding_boxes import BoundingBoxes, BoundingBoxFormat, CLAMPING_MODE_TYPE, is_rotated_bounding_format |
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.
With CLAMPING_MODE_TYPE
now being used for both bounding boxes and keypoints alike, it would make sense to move its definition out of _bounding_boxes
, e.g., directly to tv_tensors.__init__
.
|
||
class SetClampingMode(Transform): | ||
"""Sets the ``clamping_mode`` attribute of the bounding boxes for future transforms. | ||
"""Sets the ``clamping_mode`` attribute of the bounding boxes and keypoints for future transforms. |
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 could be useful to allow setting the clamping modes of bounding boxes and keypoints to different values by passing a dictionary.
For example:
SetClampingMode("soft")
sets the clamping mode of both bounding boxes and keypoints to"soft"
.SetClampingMode({tv_tensors.BoundingBoxes: "hard", tv_tensors.KeyPoints: "soft"})
sets the clamping mode of bounding boxes to"hard"
and that of keypoints to"soft"
.SetClampingMode({tv_tensors.BoundingBoxes: "hard"})
sets the clamping mode of bounding boxes to"hard"
and leaves that of keypoints unchanged.
keypoints = tv_tensors.KeyPoints( | ||
[[0, 100], [0, 100]], canvas_size=(10, 10), clamping_mode=constructor_clamping_mode | ||
) | ||
expected_clamped_output = ( |
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.
Looks like this line is redundant and can be removed.
Context
This PR proposes to add a
clamping_mode
parameter forKeyPoints
. This parameter is similar to what have been added forBoundingBoxes
(#9128). We are adding this parameter here to avoid the issue observed in #9223, where clamping modifies the position ofKeyPoints
and create a misalignment with the actual locations in the transformed image. This PR will need to be supplemented with another PR to implement a class similar toSanitizeBoundingBoxes
transform to remove non validKeyPoints
, e.g. outside of the image canvas.Implementation details
In the current proposition, we align the possible values for the
clamping_mode
parameters with what is already existing forBoundingBoxes
, that is"soft"
,"hard"
, andNone
. We would likeKeyPoints
clamping mode to be consistent with existing data structures. We propose the following behaviors for each value:"soft"
: this is the default value if not specified otherwise. This will not apply any clamping operation;None
: This will not apply any clamping operation;"hard"
: This will apply the current default transformation with each coordinates being bounded between0
andcanvas_size - 1
.We choose this behavior as we believe that we want the default behavior to avoid creating misalignment with the transformed image. In this case,
KeyPoints
are not clamped unless explicitly settingclamping_mode="hard"
.Illustration of the changes
We illustrate below how those changes impact the code.
With
clamping_mode="hard"
With
clamping_mode="soft"
orNone
Testing
Please run the following unit tests