Skip to content

Added annotation typing to mobilenet #2862

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

Merged
merged 5 commits into from
Oct 23, 2020
Merged

Conversation

frgfm
Copy link
Contributor

@frgfm frgfm commented Oct 21, 2020

Hi there!

As per #2025, annotation typing are welcome in torchvision. So, this PR focuses on torchvision.models.mobilenet this PR!

Any feedback is welcome!

@pmeier pmeier self-assigned this Oct 21, 2020
Copy link
Collaborator

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

Only incremental changes needed from the other PRs. Otherwise LGTM.

@frgfm
Copy link
Contributor Author

frgfm commented Oct 22, 2020

Only incremental changes needed from the other PRs. Otherwise LGTM.

Fixed! Although, I'm still getting an issue I didn't manage to get around:

torchvision/models/mobilenet.py:82: error: List item 1 has incompatible type
"Conv2d"; expected "ConvBNReLU"  [list-item]
                nn.Conv2d(hidden_dim, oup, 1, 1, 0, bias=False),
                ^
torchvision/models/mobilenet.py:83: error: List item 2 has incompatible type
Module; expected "ConvBNReLU"  [list-item]
                norm_layer(oup),
                ^
torchvision/models/mobilenet.py:154: error: Argument 1 to "append" of "list"
has incompatible type Module; expected "ConvBNReLU"  [arg-type]
                    features.append(block(input_channel, output_channel, s...

@fmassa
Copy link
Member

fmassa commented Oct 23, 2020

@pmeier any way of telling mypy that ConvBNReLU inherits from Module?

@pmeier
Copy link
Collaborator

pmeier commented Oct 23, 2020

I think annotating layers: List[nn.Module] = [] should be sufficient. If the annotation is left out mypy assumes the first type that is added to the list is the only one. Thus, it probably assumes that layers is List[ConvBNReLU] and thus complains.

@frgfm You can check which type mypy uses for a variable by simply inserting reveal_type(layers) into the code and run mypy over it.

@frgfm
Copy link
Contributor Author

frgfm commented Oct 23, 2020

@frgfm You can check which type mypy uses for a variable by simply inserting reveal_type(layers) into the code and run mypy over it.

Neat trick @pmeier 👌 Your suggestion is working, just pushed it!

Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

Thanks!

@fmassa fmassa merged commit d4cd0be into pytorch:master Oct 23, 2020
@frgfm frgfm deleted the mobilenet-typing branch October 23, 2020 10:38
bryant1410 pushed a commit to bryant1410/vision-1 that referenced this pull request Nov 22, 2020
* style: Added annotation typing for mmobilenet

* fix: Fixed type hinting of adaptive pooling

* refactor: Removed un-necessary import

* fix: Fixed constructor typing

* fix: Fixed list typing
vfdev-5 pushed a commit to Quansight/vision that referenced this pull request Dec 4, 2020
* style: Added annotation typing for mmobilenet

* fix: Fixed type hinting of adaptive pooling

* refactor: Removed un-necessary import

* fix: Fixed constructor typing

* fix: Fixed list typing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants