-
Notifications
You must be signed in to change notification settings - Fork 7.1k
add consistency tests for prototype container transforms #6525
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
class TestToTensorTransforms: | ||
def test_pil_to_tensor(self): | ||
prototype_transform = prototype_transforms.PILToTensor() |
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.
We don't need to create the transform for ever image.
def __init__(self, transform: Transform, *, p: float = 0.5) -> None: | ||
super().__init__(p=p) | ||
self.transform = transform | ||
class RandomApply(Compose): |
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 don't really understand why RandomApply
needs to support multiple transforms. I've re-added it here for BC, but I think we should deprecate this in favor of what our internal _RandomApplyTransform
does. If the user needs multiple transforms, they can simply wrap them in a Compose
before passing it to RandomApply
.
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 agree. Maybe, we can extend Compose
with p
argument to cover RandomApply
feature...
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 for now we should keep it and review on the future what we want to deprecate. It's true that many of the transforms can be written as a combination of other transforms. For example, the majority of the Random*
transforms that support a p
probability could have used the RandomApply
. Unfortunately dropping some of these functionalities not only breaks BC but also leads to more verbose code. So we should have a separate discussion over what should be deprecated and why.
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 ok to me, thanks @pmeier
Per title.
When designing the prototype API, we intentionally only used
torch
's random capabilities to generate parameters everywhere. However, on the legacy API there are two transforms that don't:There are two outliers:
RandomChoice
:vision/torchvision/transforms/transforms.py
Lines 565 to 567 in cc9ceb5
RandomOrder
:vision/torchvision/transforms/transforms.py
Lines 548 to 553 in cc9ceb5
For the former we can at least test for
p=0
andp=1
, but for the latter I have no idea how to test other than manually. Given that code is very simple, I want to avoid complicated mocking like we are doing in #6519.The equivalent logic on the prototype transforms is:
RandomChoice
:vision/torchvision/prototype/transforms/_container.py
Lines 66 to 69 in cc9ceb5
RandomOrder
:vision/torchvision/prototype/transforms/_container.py
Lines 79 to 84 in cc9ceb5
Basically we need to make sure that they generate the same distribution. I confirmed that they are consistent visually.
Apart from the testing, I also fixed a consistency bug in
RandomApply
.