Refactoring of the datasets #749

pmeier · 2019-02-15T12:04:13Z

I'm not sure if there is a practical reason for almost all datasets to individually define the attributes root, transform, and target_transform as well as the __repr__ method. Since I think there is no reason I've introduced a superclass VisionDataset to streamline the process. This achieves two things:

Since almost all classes have a root and target_transform attribute and all of them have a transform attribute, I think it is a good idea to move them to VisionDataset. The only two exceptions are: FakeData without root and PhotoTour without target_transform. Both cases are handled by VisionDataset if None is passed for these arguments.
I've implemted the __repr__ similar to torch.nn.Module. Thus, __repr__ always returns the name of the dataset as well as the number of samples within it. If the attributes root, transform, and target_transform are not None they are added. Additionally, the subclasses can override the extra_repr method to include information specific for that dataset.

Next to the refactoring with these changes the datasets CocoCaptions, Flickr*, Omniglot, SBU, and VOC* now also have a human-readable representation.

fmassa

Hi,

Thanks for the PR!

It looks good for the most part, I only have a couple of comments:

the inheritance pattern should be Python2 friendly for new-style classes, so using super(MyClass, self) would be preferable to super().
I'm unsure if I'd want to add the transform and target_transform to the base class. The reason is that for some tasks, we might want to apply the same random transform to both input and target. This is currently not well supported in torchvision anyway, but I'm planning a few things that would make it better.

Thoughts about 2. ?

torchvision/datasets/cifar.py

torchvision/datasets/vision.py

pmeier · 2019-02-15T15:50:26Z

2. I'm unsure if I'd want to add the `transform` and `target_transform` to the base class. The reason is that for some tasks, we might want to apply the same random transform to both input and target. This is currently not well supported in torchvision anyway, but I'm planning a few things that would make it better.

Thoughts about 2. ?

I'm not sure if I understand you correctly, but wouldn't the following work in such a case?

class CrazyDataset(VisionDataset):
    def __init__(self, root, transform=None):
        super(VisionDataset, self).__init__(root, transform, transform)
        ...

Although I've never worked with it, isn't this somewhat similar to the PhotoTour dataset? It only accepts a transform but applies this to both returned objects.

torchvision/datasets/cifar.py

fmassa · 2019-02-18T10:32:07Z

@pmeier

Although I've never worked with it, isn't this somewhat similar to the PhotoTour dataset? It only accepts a transform but applies this to both returned objects.

Yes, you could handle those cases with something like

class CrazyDataset(VisionDataset):
    def __init__(self, root, transform=None):
        super(CrazyDataset, self).__init__(root, None, None)

but then, this means that VisionDataset is not generic enough to handle everything.

For example, in #230 (comment) I mention one alternative, StandardTransform, which looks like

class StandardTransform(object):
    def __init__(self, transform, target_transform):
        self.transform = transform
        self.target_transform = target_transform

   def __call__(self, input, target):
        if self.transform:
            input = self.transform(input)
        if self.target_transform:
            target = self.target_transform(target)
        return input, target

and which could replace the transform and target_transform by a simple transforms.

Because I'm not 100% sure of such a thing, I'd prefer to have the base class be for now very dummy and basic, and then maybe add support for more things afterwards once we are sure about it. Thoughts?

pmeier · 2019-02-18T15:08:31Z

Thanks for the additional information as I wasn't aware of that discussion.

Just to be clear: you want StandardTransform only as the fallback during the transition period, correct? Later on the user should define his own transform, which jointly handles the input as well as the target, correct?

fmassa · 2019-02-18T15:41:17Z

Just to be clear: you want StandardTransform only as the fallback during the transition period, correct? Later on the user should define his own transform, which jointly handles the input as well as the target, correct?

@pmeier I'm not 100% sure about this either :-) My original idea was to have it as a fallback initially, but I still need to think a bit more about it. This is why I'd rather have VisionDataset for now be just a dummy class that takes only root as an argument.

pmeier · 2019-02-18T15:58:39Z

@fmassa Alright. I will move transform and target_transform back to the subclasses. How do you want to handle the printing? I can think of two options:

Move the repr creation of transform and target_transform into extra_repr within every dataset:

# cityscapes
def extra_repr(self):
    lines = (self._format_transform_repr(self.transform, "Transforms: ") +
             self._format_transform_repr(self.target_transform, "Target transforms: ") +
             ["Split: {split}", "Mode: {mode}", "Type: {target_type}"])
    return '\n'.join(lines).format(**self.__dict__)

Leave the repr creation within VisionDataset and set transform and target_transform to None within the constructor in case they are not defined within the subclasses:

class VisionDataset(data.Dataset):
    _repr_indent = 4

    def __init__(self, root):
        if isinstance(root, torch._six.string_classes):
            root = os.path.expanduser(root)
        self.root = root
        self.transform = None
        self.target_transform = None

    def __repr__(self):
        ...
        if self.transform is not None:
            body += self._format_transform_repr(self.transform,
                                                "Transforms: ")
        if self.target_transform is not None:
            body += self._format_transform_repr(self.target_transform,
                                                "Target transforms: ")
        ...

I would prefer option 2. since it will keep the subclasses uncluttered.

fmassa · 2019-02-18T16:02:39Z

@pmeier I'd go for option 2 as well, or by some intermediate solution like

if hasattr(self, "transform") and self.transform is not None:
    ...

so that we can avoid adding the self.transform = None to the base class

…g them within the constructor

pmeier · 2019-02-18T16:09:25Z

@fmassa It's good to have someone with experience for guidance ;)

fmassa

Thanks for the update!

There are still a few things missing I believe.

torchvision/datasets/lsun.py

torchvision/datasets/flickr.py

torchvision/datasets/coco.py

codecov-io · 2019-02-18T17:23:25Z

Codecov Report

Merging #749 into master will increase coverage by 1.89%.
The diff coverage is 46.34%.

@@            Coverage Diff             @@
##           master     #749      +/-   ##
==========================================
+ Coverage   39.64%   41.54%   +1.89%     
==========================================
  Files          29       30       +1     
  Lines        2742     2684      -58     
  Branches      430      438       +8     
==========================================
+ Hits         1087     1115      +28     
+ Misses       1581     1492      -89     
- Partials       74       77       +3

Impacted Files	Coverage Δ
torchvision/datasets/lsun.py	`20.65% <33.33%> (+1.04%)`	⬆️
torchvision/datasets/coco.py	`24.48% <33.33%> (+2.07%)`	⬆️
torchvision/datasets/voc.py	`20.38% <33.33%> (ø)`	⬆️
torchvision/datasets/cityscapes.py	`19.44% <37.5%> (+2.16%)`	⬆️
torchvision/datasets/fakedata.py	`25% <40%> (+0.75%)`	⬆️
torchvision/datasets/flickr.py	`23.45% <42.85%> (ø)`	⬆️
torchvision/datasets/phototour.py	`24% <42.85%> (+1.57%)`	⬆️
torchvision/datasets/vision.py	`43.33% <43.33%> (ø)`
torchvision/datasets/mnist.py	`30.92% <50%> (+1.72%)`	⬆️
torchvision/datasets/omniglot.py	`29.62% <50%> (ø)`	⬆️
... and 8 more

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 cd12b9a...2c68951. Read the comment docs.

pmeier · 2019-02-18T17:24:21Z

@fmassa Yep you are right, Sorry for causing more work than this should have been.

fmassa · 2019-02-19T09:46:54Z

Hi @pmeier

Lint is failing with

./torchvision/datasets/mnist.py:119:16: E126 continuation line over-indented for hanging indent

Could you address that?

Thanks!

pmeier · 2019-02-19T10:00:00Z

@fmassa Can I do these checks locally? I seems counter-intuitive to me to first commit the changes and than "hope" that the commit passes all checks.

soumith · 2019-04-04T00:56:00Z

@pmeier yes, absolutely. pip install flake8 and then flake8 .

fmassa

Thanks!

Philip Meier added 5 commits February 15, 2019 11:07

introduced new super class for all vision datasets

a1cce05

Removed root from repr if dataset has no root

5e1b1c0

reverted some overly-ambitious autoformatting

2b13ca6

reverted some overly-ambitious autoformatting

c81a7e6

added split attribute to repr of STL10 dataset

17f3969

fmassa requested changes Feb 15, 2019

View reviewed changes

torchvision/datasets/cifar.py Outdated Show resolved Hide resolved

torchvision/datasets/vision.py Outdated Show resolved Hide resolved

made Python2 friendly and more robust

e0e32ea

Merge branch 'master' into refactor_dataset_repr

434af96

TheCodez reviewed Feb 15, 2019

View reviewed changes

torchvision/datasets/cifar.py Outdated Show resolved Hide resolved

Fixed call of the superclass constructor

0773175

moved transform and target_transform back to the base classes

fbf4dfe

added check if transforms are present before printing to avoid settin…

76fd755

…g them within the constructor

fmassa requested changes Feb 18, 2019

View reviewed changes

torchvision/datasets/lsun.py Outdated Show resolved Hide resolved

torchvision/datasets/flickr.py Outdated Show resolved Hide resolved

torchvision/datasets/coco.py Outdated Show resolved Hide resolved

added missing transforms and target_transforms to base classes

bffb68e

fixed linter error

2c68951

fmassa approved these changes Apr 4, 2019

View reviewed changes

fmassa merged commit 6cabab3 into pytorch:master Apr 4, 2019

pmeier mentioned this pull request Apr 6, 2019

Added support for VisionDataset #838

Merged

pmeier deleted the refactor_dataset_repr branch April 10, 2019 06:12

Refactoring of the datasets #749

Refactoring of the datasets #749

Uh oh!

Conversation

pmeier commented Feb 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmassa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pmeier commented Feb 15, 2019

Uh oh!

Uh oh!

fmassa commented Feb 18, 2019

Uh oh!

pmeier commented Feb 18, 2019

Uh oh!

fmassa commented Feb 18, 2019

Uh oh!

pmeier commented Feb 18, 2019

Uh oh!

fmassa commented Feb 18, 2019

Uh oh!

pmeier commented Feb 18, 2019

Uh oh!

fmassa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-io commented Feb 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pmeier commented Feb 18, 2019

Uh oh!

fmassa commented Feb 19, 2019

Uh oh!

pmeier commented Feb 19, 2019

Uh oh!

soumith commented Apr 4, 2019

Uh oh!

fmassa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pmeier commented Feb 15, 2019 •

edited

Loading

codecov-io commented Feb 18, 2019 •

edited

Loading