Skip to content

Support empty target_type for CelebA dataset #1351

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
Sep 23, 2019
Merged

Conversation

dniku
Copy link
Contributor

@dniku dniku commented Sep 19, 2019

Previously, passing target_type=[] wouldn't work because of this line:

celeba = CelebA('whatever/dir', target_type=[])
celeba[0]  # IndexError: list index out of range

This is a workaround that returns only X instead of X, target if target_type is empty.

@codecov-io
Copy link

codecov-io commented Sep 19, 2019

Codecov Report

Merging #1351 into master will decrease coverage by 0.66%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1351      +/-   ##
=========================================
- Coverage   65.47%   64.8%   -0.67%     
=========================================
  Files          75      76       +1     
  Lines        5827    5955     +128     
  Branches      892     912      +20     
=========================================
+ Hits         3815    3859      +44     
- Misses       1742    1822      +80     
- Partials      270     274       +4
Impacted Files Coverage Δ
torchvision/datasets/celeba.py 19.23% <0%> (-0.51%) ⬇️
torchvision/models/_utils.py 67.44% <0%> (-21.02%) ⬇️
torchvision/models/densenet.py 77.85% <0%> (-9.06%) ⬇️
torchvision/datasets/hmdb51.py 27.65% <0%> (ø) ⬆️
torchvision/io/__init__.py 100% <0%> (ø) ⬆️
torchvision/datasets/kinetics.py 34.78% <0%> (ø) ⬆️
torchvision/datasets/ucf101.py 25% <0%> (ø) ⬆️
torchvision/io/_video_opt.py 27.02% <0%> (ø)
torchvision/models/segmentation/_utils.py 80% <0%> (+0.83%) ⬆️

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 f677ea3...43414fa. 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!

What do you think about always returning image, target, but target potentially being None if you pass an empty list?

I'm not sure I'd like to see the return values of datasets to change depending on a constructor argument. This has caused some issues already, like #1273

@dniku
Copy link
Contributor Author

dniku commented Sep 19, 2019

I had some doubts about the interface: in addition to X and X, None it is also possible to return X, tuple(). I'm not sure which option is best. What would you suggest?

@fmassa
Copy link
Member

fmassa commented Sep 20, 2019

I'd say just return None. If we always returned tuple I'd say that tuple might be better, but in the case of a single output option this is not the case.

@dniku
Copy link
Contributor Author

dniku commented Sep 21, 2019

Amended code and docstring. Please re-review.

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 have a comment that I think would minimize the changes in the PR, let me know what you think

return X, target
if not self.target_type:
return X, None
else:
Copy link
Member

Choose a reason for hiding this comment

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

I would maybe simplify the changes in this PR like the following, and only change the following places:

if target:
    target = tuple(target) if len(target) > 1 else target[0]
    
    if self.target_transform is not None:
        target = self.target_transform(target)
else:
    target = None

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had doubts about keeping the diff small vs keeping the logic straightforward. Amended the commit following your advice.

Also, added a check for a meaningless combination of parameters (empty target_type with nonempty target_transform).

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.

LGTM, thanks!

@fmassa fmassa merged commit 76c04d6 into pytorch:master Sep 23, 2019
@fmassa fmassa mentioned this pull request Sep 30, 2019
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