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

add typehints for torchvision.datasets.mnist #2532

Merged
merged 1 commit into from
Aug 3, 2020

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Jul 31, 2020

No description provided.

@codecov
Copy link

codecov bot commented Jul 31, 2020

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2532   +/-   ##
=======================================
  Coverage   70.68%   70.68%           
=======================================
  Files          94       94           
  Lines        8017     8017           
  Branches     1276     1275    -1     
=======================================
  Hits         5667     5667           
  Misses       1945     1945           
  Partials      405      405           
Impacted Files Coverage Δ
torchvision/datasets/mnist.py 54.33% <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 6db1569...9ca9221. Read the comment docs.

"""Read a SN3 file in "Pascal Vincent" format (Lush file 'libidx/idx-io.lsh').
Argument may be a filename, compressed filename, or file object.
"""
# typemap
if not hasattr(read_sn3_pascalvincent_tensor, 'typemap'):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure what the original intention was to assign this dict to the function in the first run instead as a constant on the outside. I didn't manage to annotate it so that mypy didn't complain, so I moved it outside.

Copy link
Member

Choose a reason for hiding this comment

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

It's a micro-optimization, but It's ok to change it as you did. I would even put it inside the function itself, so that it doesn't appear in the global namespace of mnist.

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 3f70e3c into pytorch:master Aug 3, 2020
@pmeier pmeier deleted the typehints-mnist branch August 3, 2020 11:22
bryant1410 pushed a commit to bryant1410/vision-1 that referenced this pull request Nov 22, 2020
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.

None yet

2 participants