Skip to content
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.

Sign up for GitHub

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

Jump to bottom

allow nn.ModuleList in RandomApply #7197

Merged
merged 6 commits into from
Feb 9, 2023
Merged

allow nn.ModuleList in RandomApply #7197

merged 6 commits into from
Feb 9, 2023

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Feb 8, 2023 •
edited by pytorch-bot bot
Loading

This is allowed in v1 for JIT scriptability:

vision/torchvision/transforms/transforms.py

Lines 495 to 502 in 1120aa9

.. note::
In order to script the transformation, please use ``torch.nn.ModuleList`` as input instead of list/tuple of
transforms as shown below:
>>> transforms = transforms.RandomApply(torch.nn.ModuleList([
>>> transforms.ColorJitter(),
>>> ]), p=0.3)
>>> scripted_transforms = torch.jit.script(transforms)

cc @vfdev-5 @bjuncek

@pmeier pmeier requested review from NicolasHug and vfdev-5 February 8, 2023 21:01
pmeier
pmeier commented Feb 8, 2023
View reviewed changes
torchvision/prototype/transforms/_container.py
class RandomApply(Compose):
def __init__(self, transforms: Sequence[Callable], p: float = 0.5) -> None:
super().__init__(transforms)
class RandomApply(Transform):
Copy link
Collaborator Author

@pmeier pmeier Feb 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We inherited from Compose before, because both shared the same constructor as well as extra_repr. With this patch, the constructor is different and thus I decided to just copy paste extra_repr.

@pmeier pmeier mentioned this pull request Feb 8, 2023
Draft
NicolasHug
NicolasHug approved these changes Feb 9, 2023
View reviewed changes
Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Philip, LGTM, a small unit test would be nice before merging if we have time

@pmeier pmeier merged commit d75a524 into pytorch:main Feb 9, 2023
@pmeier pmeier deleted the random-apply-module-list branch February 9, 2023 10:29
facebook-github-bot pushed a commit that referenced this pull request Mar 28, 2023
@NicolasHug @facebook-github-bot
[fbsync] allow nn.ModuleList in RandomApply (#7197)
0afc675 
Reviewed By: vmoens

Differential Revision: D44416259

fbshipit-source-id: cbd7c731f37f13b3bdb4d117225910fa019eac60
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NicolasHug NicolasHug NicolasHug approved these changes

@vfdev-5 vfdev-5 Awaiting requested review from vfdev-5

Assignees
No one assigned
Labels
cla signed module: transforms prototype
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pmeier @NicolasHug @facebook-github-bot