Skip to content

Test for SVHN #1086

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
Jul 9, 2019
Merged

Test for SVHN #1086

merged 5 commits into from
Jul 9, 2019

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Jul 4, 2019

This moves the cast from numpy.ndarray to PIL.Image from __getitem__() to __init__() for the CIFAR10, CIFAR10, and SVHN dataset. Additionally, this adds a dataset test for the SVHN dataset.

@codecov-io
Copy link

codecov-io commented Jul 4, 2019

Codecov Report

Merging #1086 into master will increase coverage by 0.21%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1086      +/-   ##
==========================================
+ Coverage   64.66%   64.88%   +0.21%     
==========================================
  Files          68       68              
  Lines        5406     5411       +5     
  Branches      829      831       +2     
==========================================
+ Hits         3496     3511      +15     
+ Misses       1661     1643      -18     
- Partials      249      257       +8
Impacted Files Coverage Δ
torchvision/transforms/transforms.py 80.55% <0%> (-1.24%) ⬇️
torchvision/models/densenet.py 86.79% <0%> (ø) ⬆️
torchvision/models/mnasnet.py 82.71% <0%> (ø) ⬆️
torchvision/models/vgg.py 89.55% <0%> (ø) ⬆️
torchvision/models/resnet.py 88.27% <0%> (ø) ⬆️
torchvision/models/detection/transform.py 78.3% <0%> (+0.2%) ⬆️
torchvision/datasets/svhn.py 67.3% <0%> (+32.69%) ⬆️

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 7d8cc19...a90324a. 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 a lot for the PR!

The new tests look great!

I have a question regarding passing the creation of the PIL images in the constructor: does it speed up significantly the __getitem__ step? Because this is a potentially breaking change, as users could have accessed the .data field beforehand.
So it would be good to know how much slowdown this represents.

@pmeier
Copy link
Collaborator Author

pmeier commented Jul 5, 2019

So it would be good to know how much slowdown this represents.

It takes ~80µs per image (on my machine) for the conversion, thus making the performance advantage negligible. I would argue that the performance was never the decisive factor for this PR. I simply found it odd that we keep the data as numpy.ndarray while we internally use PIL as image backend. But then I'm from a prototypy world and keep forgetting that BC could be more important.

If you think BC is more important here, I'm happy to roll back the changes and only keep the test for SVHN.

@fmassa
Copy link
Member

fmassa commented Jul 9, 2019

@pmeier I think it's important to keep BC until we have a clear story about what are the fields of datasets that users can use, and those that they shouldn't rely on. Your issue on unifying datasets is a great start, and I'll make some comments today on it.

If you could revert the BC changes and keep only the test for SVHN for now, this would be ready to merge

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 a lot!

@pmeier pmeier changed the title Removing casting images to PIL at runtime for CIFAR and SVHN Test for SVHN Jul 9, 2019
@fmassa fmassa merged commit 8e60cf4 into pytorch:master Jul 9, 2019
@pmeier pmeier deleted the np_to_pil branch July 26, 2019 10:47
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