add prototype transforms that use the prototype dispatchers #5418
Conversation
Conflicts: torchvision/prototype/transforms/__init__.py
💊 CI failures summary and remediationsAs of commit 7799069 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
Conflicts: torchvision/prototype/transforms/_augment.py torchvision/prototype/transforms/_auto_augment.py torchvision/prototype/transforms/_geometry.py
| @@ -0,0 +1,184 @@ | |||
| import itertools | |||
There was a problem hiding this comment.
These test are just a quick and dirty implementation to see if the transforms don't error when called with the supported inputs.
| raise ValueError("Scale should be between 0 and 1") | ||
| if p < 0 or p > 1: | ||
| raise ValueError("Random erasing probability should be between 0 and 1") | ||
| # TODO: deprecate p in favor of wrapping the transform in a RandomApply |
There was a problem hiding this comment.
This TODO is probably left from earlier when you used more abstraction. Is this still relevant given the changes?
There was a problem hiding this comment.
Yes, this is still relevant. The idea is to remove all probability arguments from the transforms in favor of wrapping them in a RandomApply transform. In this example this means
transforms.RandomErasing(p=..., ...)would change to
transforms.RandomApply(
transforms.RandomErasing(...),
p=...,
)Another example is to no longer have a RandomHorizontalFlip. The term "Random" in the name of a transform should mean "random parameters sampled at runtime".
There was a problem hiding this comment.
I see. Let's discuss this in a separate issue. I have concerns that it will overcomplicate the definition of transforms as for literally every transform you will need 2. So let's take this on a separate thread.
There was a problem hiding this comment.
I think the proposed solution is a good candidate. I left one nit and one comment which I would address on a separate PR (to reduce the complexity of this one).
Also Have you checked that all transforms work the way we expect them to work? If not, we should create an issue to review them and verify individually but I wouldn't block this merge as it focuses on the API rather than the functionality.
…5418) Summary: * add prototype transforms that use the prototype dispatchers Reviewed By: jdsgomes Differential Revision: D34475309 fbshipit-source-id: 8366d044b8e118c7c360bfd6d828ef87c3055ced
| # Backward compatibility with integer value | ||
| if isinstance(interpolation, int): | ||
| warnings.warn( | ||
| "Argument interpolation should be of type InterpolationMode instead of int. " | ||
| "Please, use InterpolationMode enum." | ||
| ) | ||
| interpolation = _interpolation_modes_from_int(interpolation) |
There was a problem hiding this comment.
I think we can drop the support for int in prototype. Otherwise, we have to make this consistent (for example Resize class does not do that)
This PR re-adds the transformations removed in #5407 to use the dispatchers that were introduced in that PR. Since we have the high-level functional API (dispatchers), most transforms only need to subclass from the
Transformbase class and set the_DISPATCHERattribute. In case the transformation takes additional parameters,__init__andget_paramshas to be overwritten. In case the parameters are constant, one can also subclass fromConstantParamTransformto make this even more concise.