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.

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

Added annotation typing to squeezenet #2865

Merged
merged 2 commits into from
Oct 22, 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.squeezenet this PR!

Any feedback is welcome!

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

codecov bot commented Oct 21, 2020

Codecov Report

Merging #2865 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2865   +/-   ##
=======================================
  Coverage   73.36%   73.36%           
=======================================
  Files          99       99           
  Lines        8787     8787           
  Branches     1387     1387           
=======================================
  Hits         6447     6447           
  Misses       1916     1916           
  Partials      424      424           
Impacted Files Coverage Δ
torchvision/models/squeezenet.py 86.53% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 57b653c...dc2ad02. Read the comment docs.

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 for the PR!

I have one comment, let me know what you think

x = self.features(x)
x = self.classifier(x)
return torch.flatten(x, 1)


def _squeezenet(version, pretrained, progress, **kwargs):
def _squeezenet(version: str, pretrained: bool, progress: bool, **kwargs) -> SqueezeNet:
Copy link
Member

Choose a reason for hiding this comment

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

nit: you are not typing kwargs here.

Also, should we start enabling mypy tests for each one of the files that we enable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch! Just fixed it, that would be good, my local mypy run didn't catch this.
But I'm unsure about how to proceed with this? Any suggestion about the mypy.ini?

Copy link
Member

Choose a reason for hiding this comment

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

According to @pmeier, the only tests currently disabled are in

vision/mypy.ini

Lines 15 to 25 in aa4cf03

[mypy-torchvision.models.detection.*]
ignore_errors = True
[mypy-torchvision.models.densenet.*]
ignore_errors = True
[mypy-torchvision.models.quantization.*]
ignore_errors = True
so I'm not sure why it wasn't caught before.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This did not show up since we don't enforce annotations yet. If we hit a significant coverage we should enable disallow_untyped_defs. Unfortunately, mypy makes no difference between an untyped and partially typed definition and thus this is disabled for now.

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 65591f1 into pytorch:master Oct 22, 2020
@frgfm frgfm deleted the squeezenet-typing branch October 22, 2020 15:06
bryant1410 pushed a commit to bryant1410/vision-1 that referenced this pull request Nov 22, 2020
* style: Added annotation typing for squeezenet

* feat: Added typing for kwargs
vfdev-5 pushed a commit to Quansight/vision that referenced this pull request Dec 4, 2020
* style: Added annotation typing for squeezenet

* feat: Added typing for kwargs
@frgfm frgfm mentioned this pull request Oct 27, 2022
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