-
Notifications
You must be signed in to change notification settings - Fork 7.1k
[proto] Added some transformations and fixed type hints #6245
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
Conversation
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.
Thanks @vfdev-5 for the PR. I've added a few comments, let me know your thoughts.
|
||
def _get_params(self, sample: Any) -> Dict[str, Any]: | ||
# vfdev-5: techically, this op can work on bboxes/segm masks only inputs without image in samples | ||
# What if we have multiple images/bboxes/masks of different sizes ? | ||
# TODO: let's support bbox or mask in samples without image |
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.
Correct. This logic was added before you land kernels for handling crops BBoxes and masks. Now we can review this transform. It's worth noting that passing bboxes or masks without image doesn't make a lot of sense in practice because this will cause desyncing between the image and the targets. This is something that was brought up many times as a potential feature (passing only bboxes) but so far we haven't found a compelling example on where this can be used in practice. Happy to review this if we do, but for now we can assume that an image (or video coming up) will be available to give us access to the width/height here.
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.
Not a practical use-case, but we can't test this transform on bboxes/masks without an image :)
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'm curious to see if this creates issues such as having no access to specific meta-data you would need from the image. If you can do this without them, we can chat adding it. But if you have to add lots of workarounds to support it, we should find a real-world use-case. I think the test use-case can be achieved by passing an equivalent image.
Updated code according to the review
…nto proto-transforms-api-p3
Go back with if-return/elif-return/else-return
a16e61d
to
42b4bf3
Compare
883a525
to
12adbe8
Compare
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.
LGTM, as discussed offline the has_all()
and has_any()
methods need to change to support some additional requirement but this can happen on a separate PR.
I'll merge this PR to main and then work with proto-transforms-api branch |
Hey @vfdev-5! You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py |
Summary: * Another attempt to add transforms * Fixed padding type hint * Fixed fill arg for pad and rotate, affine * code formatting and type hints for affine transformation * Fixed flake8 * Updated tests to save and load transforms * Fixed code formatting issue * Fixed jit loading issue * Restored fill default value to None Updated code according to the review * Added tests for rotation, affine and zoom transforms * Put back commented code * Random erase bypass boxes and masks Go back with if-return/elif-return/else-return * Fixed acceptable and non-acceptable types for Cutmix/Mixup * Updated conditions for _BaseMixupCutmix Reviewed By: jdsgomes Differential Revision: D37993418 fbshipit-source-id: 900faa217ce7cc0297eaa62671fe0518e8b4bc83
Description:
padding
arg type hintfill
arg type hintChangedfill
arg type hint, default from None to 0