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 Caltech101, Caltech256, and CelebA #775

Merged
merged 10 commits into from
Mar 7, 2019
Merged

Conversation

bryanhe
Copy link
Contributor

@bryanhe bryanhe commented Mar 3, 2019

No description provided.

@codecov-io
Copy link

codecov-io commented Mar 3, 2019

Codecov Report

Merging #775 into master will decrease coverage by 2.85%.
The diff coverage is 17.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #775      +/-   ##
==========================================
- Coverage   40.99%   38.13%   -2.86%     
==========================================
  Files          29       32       +3     
  Lines        2747     3126     +379     
  Branches      432      487      +55     
==========================================
+ Hits         1126     1192      +66     
- Misses       1542     1855     +313     
  Partials       79       79
Impacted Files Coverage Δ
torchvision/datasets/__init__.py 100% <100%> (ø) ⬆️
torchvision/datasets/celeba.py 14.73% <14.73%> (ø)
torchvision/datasets/caltech.py 21.13% <21.13%> (ø)
torchvision/datasets/utils.py 35.1% <9.67%> (-13.29%) ⬇️
torchvision/models/__init__.py 100% <0%> (ø) ⬆️
torchvision/models/googlenet.py 15.87% <0%> (ø)

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 6cc4970...5bf4ecf. 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.

Hi,

Thanks for the PR and sorry for the delay in reviewing it.

It generally looks good, I have a few comments.

Also, in a more general question: is Caltech101 and Caltech256 still used for papers nowadays? If I remember properly, you could get very good classification results by just classifying on the average color in the image.

torchvision/datasets/caltech.py Outdated Show resolved Hide resolved
torchvision/datasets/celeba.py Show resolved Hide resolved
torchvision/datasets/utils.py Outdated Show resolved Hide resolved
@bryanhe
Copy link
Contributor Author

bryanhe commented Mar 7, 2019

I think all of the issues should be resolved now.

I don't think many papers only use Caltech101/256, but I've seen it used for domain adaptation experiments in somewhat recent papers, such as http://papers.nips.cc/paper/6757-learning-multiple-tasks-with-deep-relationship-networks (2017), along with a few papers that it cites.

Do you think some of those (Office-Home and ImageCLEF) are worth including?

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.

I think this looks almost ready to be merged, thanks!

I have only a few minor nits about using a legacy torch.LongTensor construct, otherwise this is good to be merged

torchvision/datasets/celeba.py Outdated Show resolved Hide resolved
@fmassa
Copy link
Member

fmassa commented Mar 7, 2019

I haven't myself seen / used Office-Home and ImageCLEF be used much, but if you think it would be valuable to have them in torchvision, if you could send a PR adding them it would be great

@bryanhe
Copy link
Contributor Author

bryanhe commented Mar 7, 2019

Last comment is resolved now.

I'll think about whether those two are worth including.
I was also going to add in a few other datasets (DeepFashion and ADE2K are fairly popular, so I assume they're worth adding?). Does it make sense to PR each on separately, or just block them together?

@fmassa
Copy link
Member

fmassa commented Mar 7, 2019

Separate PRs are better I think.

Thanks!

@fmassa fmassa merged commit 9d9f48a into pytorch:master Mar 7, 2019
@bryanhe bryanhe deleted the celeb branch March 7, 2019 23:22
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.

4 participants