Repository: mila-udem/fuel
Pull request #156 (2015-06-08 17:45:59)

Add CalTech101 Silhouettes dataset
Binary dataset created by Benjamin M Marlin based on the CalTech101 dataset:
  https://people.cs.umass.edu/~marlin/data.shtml

- [x] Documentation
- [x] Test 
- [x] Handle 16x16 and 28x28 datasets

vdumoulin: Great! I'll have a look this afternoon.
vdumoulin: I did a quick first pass at reviewing your PR. Aside from my comments it's looking pretty good!

We'll need to have unit tests for the new dataset, but I see you already have that noted in your TODO list. Ping me when you're ready for a second review pass.

Thanks!
jbornschein: Thanks for the first typos!


I have to admit I find it quite annoying to write tests that generate mock datasets for these  small datasets . These two dataset files together take less than 1.5 M. To me It seems more efficient to just download the data and convert the real thing (efficiency in terms of programmer efficiency :)  ) -- and it would make the test cover the real thing.
vdumoulin: I guess that's to be evaluated on a per-dataset basis. It also depends on how much you value being able to run tests offline. Worst case scenario, the test fails and we get a false negative.

In that specific case, I agree that it could be worth it to just download the dataset and test directly on it. Could you tell me how much time the test takes to complete using the real dataset?
jbornschein: time fuel-download caltech101_silhouettes 16   ->  real	0m0.667s
time fuel-convert caltech101_silhouettes 16  -> real	0m0.424s

... so download and conversion together take less than 1.2 seconds (for the 16x16 dataset)


vdumoulin: I'm sold, then!
jbornschein: Ok, I guess this addresses all issues. The unittest now downloads and converts the 16x16 dataset and removes it again afterwards.

Coverage decreased slightly; I guess mostly because the argument subparsers are not tested. 
jbornschein: Ok, I guess this addresses all issues. 

The unittests now actually downloads and converts the 16x16 dataset. Test coverage decreased slightly by 0.05% -- Probably because the argument-subparsers are not being tested.

Anything else?
vdumoulin: Regarding test coverage, here are some suggestions:
* Testing that the downloader `fill_subparser` function sets the right URL and file name and that the converter `fill_subparser` sets the right converter function and output file (see [here](https://github.com/mila-udem/fuel/blob/master/tests/test_downloaders.py#L86-L97), for example)
* Asserting that `silhouettes_downloader` and `convert_silhouettes` raise a `ValueError` when `size` is other than 16 or 28
* Asserting that `convert_silhouettes` raises an error when the input file is not present.

I think we should also have a thin dataset wrapper around the HDF5 file like other datasets have.

Other than that, I think we'll be ready to merge. Thanks a lot!
jbornschein: Doh! The dataset wrapper was there all along -- just not added to the index. There it is!

Also increased the test coverage with almost all the cases you proposed.
bartvm: You probably just need to add your dataset to this line so that it's downloaded and cached on Travis for the tests: https://github.com/mila-udem/fuel/blob/master/.travis.yml#L40
jbornschein: Ah, ok, thanks!
jbornschein: I tried to add it to .travis-data.sh, but (of course) it won't work because both the downloader and the converter for CalTach101 Silhouettes need a second parameter, the size. 

Even when I hack it so that travis-data.sh takes e.g. "caltech101_silhouettes 16" as an argument and expands it accordingliy it won't work, because the caching mechanism does not now the hdf5 filename. The same thing would happen when the downloader/converter operate on both sizes witout size parameter.
bartvm: > because the caching mechanism does not now the hdf5 filename. 

I don't understand this. Travis CI should cache the entire data directory, regardless of the names of the files in there, so as long as it manages to download and convert, you're good.
jbornschein: It would download and convert it on every run because the condition 

https://github.com/mila-udem/fuel/blob/master/.travis-data.sh#L7-11

is always fullfilled. But.... yes... with this super-small dataset it probably doesn't matter.
bartvm: Right. I guess we could change the script so that it checks if any file matching `caltech101_silhouettes*` exists. It's reasonable to assume that even if a downloader/converter takes arguments, the files it produces are of the form `dataset*.hdf5`. Technically this could fail on Travis if we want to add a second `caltech101_silhouettes` file, but in that case we can just manually clear the cache.
vdumoulin: Alternatively, would it be possible to make that script more flexible?
jbornschein: Related to this -- but in principle a seperate issue:

I found it a bit wired that the downloader/converter are given the output filename of the file they are supposed to create (instead just the directory, and they choose and communicate the filesnames they created back to the rest of the library).

This also creates an isse with SVHN and with Silhouettes: They both create two output files and the output-filename is a pattern: 
https://github.com/mila-udem/fuel/blob/master/fuel/converters/svhn.py#L347 

So the code that wants to add version information does is on an non-existing file:
https://github.com/mila-udem/fuel/blob/master/bin/fuel-convert#L47 
vdumoulin: Regarding how file names are chosen, this is to give the user the freedom to choose under which name the dataset will be saved, which may prove useful in case of name conflicts (e.g. same dataset, but different preprocessing applied).

Regarding the pattern in the output file, good catch, I didn't think of that and was wondering why I was left with `svhn_format_{}.hdf5` on my system after conversion. That is problematic.

The problem I was trying to solve is how to have a default value in the argument parser that depends on the value of another argument. I'm open to suggestions if there's a way to do that in `argparse` directly. I'll make another Google search to check if someone else faced the same issue and solved it.
jbornschein: To make sure the user can keep multiple versions of a dataset around one could also just make it mandatory that each converter takes a "version-tag" or "variant-tag" as argument that it has to be added to the filename(s). I think this would give us better support for datasets that provide multiple files in the filesystem. 

Related to this PR: Travis is failing because the blocks tests fail (the fuel tests are green). It seems the blocks tests call an old/installed version of fuel-download that does not know about the Silhouettes dataset yet. Not sure if there is anything I can do about this.

vdumoulin: I'm not sure I understand your "multiple files in the filesystem" argument: at the moment `fuel-convert` deals with datasets that have a single HDF5 file as their data source. Could you elaborate on that?
vdumoulin: *(Also, I can't figure out why Travis fails on the Blocks tests)*
jbornschein: At least Silhouettes and TorontoFaceDatabase come in two sizes -- and it is convenient to store them as two different hdf5 files. I'm about to add "UCI binary datasets" -- which are essentially 8 different really small datasets with differend splits, shapes and number of datasets. 
For all these it seems unneccesary complicated to squeeze them into one hdf5 file.

So I'm essentially proposing that fuel-convert should be able to deal with datasets that have multiple hdf5 files.

SVHN seems to be similar because you also chose to generate seperate files for 1 and 2; but there the datasets are large and a user probably really wants to choose which one she wants to convert. 
bartvm: I think what his happening is that Blocks is installed first, which installs the master branch of Fuel as a dependency. When `pip install .` is called, it will find that is Fuel is already installed (because there is --upgrade). This means that Blocks is being tested against the master branch, not the PR branch. We should probably move the `pip install .` line before the Blocks install block
vdumoulin: Okay, I can definitely see the use case for more than one HDF5 file being tied to a particular dataset class.

However, I still think we can manage that without the converters hard-coding a name for each of those datasets. Consider this, for instance:

``` bash
for which_format in "format_1 format_2 format_3"; do
  fuel-download my_dataset $format_1
  fuel-convert my_dataset $format_1 -o my_custom_name_$format_1
  fuel-download my_dataset $format_1 --clear
```

Downloading and converting only what you need remains straightforward, even if you'd like to manage multiple formats at once.
jbornschein: Sure, the current approach works. To me it just seems that the filename is part of the contract between the converter and the H5PyDataset wrapper -- so for most off-the-shelf datasets these filenames should be chosen by whoever implements the converter and the dataset-wrapper.

Would you assume that we always generate one file per `fuel-convert` call? For the UCI datasets this would mean that the user indeed calls `fuel-download` and `fuel-convert` for each of the 8 tiny datasets?




 

vdumoulin: At the moment this is my assumption, yes. Like I mentioned earlier, this can be automated in the shell through a for-loop.

I'm not familiar with the UCI datasets; do the 8 datasets always go together, or are they used in a mutually exclusive fashion? I could see why you'd like to generate the datasets all at once if it doesn't make sense to use only one of the datasets. For datasets like SVHN of TFD, it would make more sense to generate one dataset at a time.
jbornschein: For UCI, one by one would be fine because they can be used individually. It's just that nobody would take you serious if you report only the results for one of them. (Well, even reporting all 8 will probably not enable you to make any strong claim about anything). 

The one file per `fuel-convert` call just felt like a unnecessary restriction related to the .hdf file naming that came up earlier. I don't think there is any (significant) pressure to change it.

vdumoulin: @jbornschein I opened a pull request that should fix some of the problems you outlined. We would still be maintaining the single output file per call limit, but the mechanism through which default names are specified would be shifted from `fuel-convert` to the converter itself, which would also be responsible for returning the path pointing to the converted dataset. This fixes the 'pattern' bug you mentioned (because `fuel-convert` now uses the path returned by the converter rather than the value in `args`) and makes it less awkward to specify a default file name conditioned on command-line argument values.

I'm reluctant to allow more than one file to be saved at a time, because I have a hard time seeing how to implement that logic while remaining as flexible as we are right now (i.e. giving the user control over which directory/filename to save to).
vdumoulin: Forgot to link to the PR: #158 .
jbornschein: I fixed some minor issues and adapted it to the new H5PYDataset interface -- I think it is in such a shape that it could be merged. Anybody cares to take another look?
vdumoulin: Once my comment has been addressed it'll be ready to merge.

Patch:
diff --git a/.travis-data.sh b/.travis-data.sh
index 2ee5074..18a4ec9 100755
--- a/.travis-data.sh
+++ b/.travis-data.sh
@@ -5,9 +5,9 @@ set -ev
 
 function download {
   if [ ! -f $FUEL_DATA_PATH/$1.hdf5 ]; then
-    fuel-download $1
-    fuel-convert $1
-    fuel-download $1 --clear
+    fuel-download $@
+    fuel-convert $@
+    fuel-download $@ --clear
   fi
 }
 
diff --git a/.travis.yml b/.travis.yml
index 62b8c2b..33c7c24 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -38,7 +38,7 @@ install:
         pip install . -r requirements.txt
       fi
 script:
-  - ./.travis-data.sh mnist binarized_mnist cifar10 cifar100
+  - ./.travis-data.sh mnist binarized_mnist "caltech101_silhouettes 16" cifar10 cifar100
   - function fail { export FAILED=1; }
   - |
       if [[ $TESTS == 'blocks' ]]; then
diff --git a/fuel/converters/__init__.py b/fuel/converters/__init__.py
index 259aceb..629a291 100644
--- a/fuel/converters/__init__.py
+++ b/fuel/converters/__init__.py
@@ -11,6 +11,7 @@ command-line arguments, and is expected to download the required files.
 
 """
 from fuel.converters import binarized_mnist
+from fuel.converters import caltech101_silhouettes
 from fuel.converters import cifar10
 from fuel.converters import cifar100
 from fuel.converters import mnist
@@ -19,6 +20,7 @@ from fuel.converters import svhn
 __version__ = '0.2'
 all_converters = (
     ('binarized_mnist', binarized_mnist.fill_subparser),
+    ('caltech101_silhouettes', caltech101_silhouettes.fill_subparser),
     ('cifar10', cifar10.fill_subparser),
     ('cifar100', cifar100.fill_subparser),
     ('mnist', mnist.fill_subparser),
diff --git a/fuel/converters/caltech101_silhouettes.py b/fuel/converters/caltech101_silhouettes.py
new file mode 100644
index 0000000..c8b8328
--- /dev/null
+++ b/fuel/converters/caltech101_silhouettes.py
@@ -0,0 +1,76 @@
+import os
+import h5py
+
+from scipy.io import loadmat
+
+from fuel.converters.base import fill_hdf5_file, MissingInputFiles
+
+
+def convert_silhouettes(size, directory, output_directory,
+                        output_file=None):
+    """ Convert the CalTech 101 Silhouettes Datasets.
+
+    Parameters
+    ----------
+    size : {16, 28}
+        Convert either the 16x16 or 28x28 sized version of the dataset.
+    directory : str
+        Directory in which the required input files reside.
+    output_file : str
+        Where to save the converted dataset.
+
+    """
+    if size not in (16, 28):
+        raise ValueError('size must be 16 or 28')
+
+    if output_file is None:
+        output_file = 'caltech101_silhouettes{}.hdf5'.format(size)
+    output_file = os.path.join(output_directory, output_file)
+
+    input_file = 'caltech101_silhouettes_{}_split1.mat'.format(size)
+    input_file = os.path.join(directory, input_file)
+
+    if not os.path.isfile(input_file):
+        raise MissingInputFiles('Required files missing', [input_file])
+
+    with h5py.File(output_file, mode="w") as h5file:
+        mat = loadmat(input_file)
+
+        train_features = mat['train_data'].reshape([-1, 1, size, size])
+        train_targets = mat['train_labels']
+        valid_features = mat['val_data'].reshape([-1, 1, size, size])
+        valid_targets = mat['val_labels']
+        test_features = mat['test_data'].reshape([-1, 1, size, size])
+        test_targets = mat['test_labels']
+
+        data = (
+            ('train', 'features', train_features),
+            ('train', 'targets', train_targets),
+            ('valid', 'features', valid_features),
+            ('valid', 'targets', valid_targets),
+            ('test', 'features', test_features),
+            ('test', 'targets', test_targets),
+        )
+        fill_hdf5_file(h5file, data)
+
+        for i, label in enumerate(('batch', 'channel', 'height', 'width')):
+            h5file['features'].dims[i].label = label
+
+        for i, label in enumerate(('batch', 'index')):
+            h5file['targets'].dims[i].label = label
+    return (output_file,)
+
+
+def fill_subparser(subparser):
+    """Sets up a subparser to convert CalTech101 Silhouettes Database files.
+
+    Parameters
+    ----------
+    subparser : :class:`argparse.ArgumentParser`
+        Subparser handling the `caltech101_silhouettes` command.
+
+    """
+    subparser.add_argument(
+        "size", type=int, choices=(16, 28),
+        help="height/width of the datapoints")
+    subparser.set_defaults(func=convert_silhouettes)
diff --git a/fuel/datasets/__init__.py b/fuel/datasets/__init__.py
index 360e72b..8b7d911 100644
--- a/fuel/datasets/__init__.py
+++ b/fuel/datasets/__init__.py
@@ -6,6 +6,7 @@ from fuel.datasets.hdf5 import H5PYDataset
 from fuel.datasets.binarized_mnist import BinarizedMNIST
 from fuel.datasets.cifar10 import CIFAR10
 from fuel.datasets.cifar100 import CIFAR100
+from fuel.datasets.caltech101_silhouettes import CalTech101Silhouettes
 from fuel.datasets.mnist import MNIST
 from fuel.datasets.svhn import SVHN
 from fuel.datasets.text import TextFile
diff --git a/fuel/datasets/caltech101_silhouettes.py b/fuel/datasets/caltech101_silhouettes.py
new file mode 100644
index 0000000..7fdeee1
--- /dev/null
+++ b/fuel/datasets/caltech101_silhouettes.py
@@ -0,0 +1,42 @@
+# -*- coding: utf-8 -*-
+import os
+
+from fuel import config
+from fuel.datasets import H5PYDataset
+
+
+class CalTech101Silhouettes(H5PYDataset):
+    u"""CalTech 101 Silhouettes dataset.
+
+    This dataset provides the `split1` train/validation/test split of the
+    CalTech101 Silhouette dataset prepared by Benjamin M. Marlin [MARLIN].
+
+    This class provides both the 16x16 and the 28x28 pixel sized version.
+    The 16x16 version contains 4082 examples in the training set, 2257
+    examples in the validation set and 2302 examples in the test set. The
+    28x28 version contains 4100, 2264 and 2307 examples in the train, valid
+    and test set.
+
+    .. [MARLIN] https://people.cs.umass.edu/~marlin/data.shtml
+
+    Parameters
+    ----------
+    which_sets : tuple of str
+        Which split to load. Valid values are 'train', 'valid' and 'test'.
+    size : {16, 28}
+        Either 16 or 28 to select the 16x16 or 28x28 pixels version
+        of the dataset (default: 28).
+
+    """
+    def __init__(self, which_sets, size=28, load_in_memory=True, **kwargs):
+        if size not in (16, 28):
+            raise ValueError('size must be 16 or 28')
+
+        self.filename = 'caltech101_silhouettes{}.hdf5'.format(size)
+        super(CalTech101Silhouettes, self).__init__(
+            self.data_path, which_sets=which_sets,
+            load_in_memory=load_in_memory, **kwargs)
+
+    @property
+    def data_path(self):
+        return os.path.join(config.data_path, self.filename)
diff --git a/fuel/downloaders/__init__.py b/fuel/downloaders/__init__.py
index cd39908..8640a82 100644
--- a/fuel/downloaders/__init__.py
+++ b/fuel/downloaders/__init__.py
@@ -7,6 +7,7 @@ Download functions accept two arguments:
 
 """
 from fuel.downloaders import binarized_mnist
+from fuel.downloaders import caltech101_silhouettes
 from fuel.downloaders import cifar10
 from fuel.downloaders import cifar100
 from fuel.downloaders import mnist
@@ -14,6 +15,7 @@ from fuel.downloaders import svhn
 
 all_downloaders = (
     ('binarized_mnist', binarized_mnist.fill_subparser),
+    ('caltech101_silhouettes', caltech101_silhouettes.fill_subparser),
     ('cifar10', cifar10.fill_subparser),
     ('cifar100', cifar100.fill_subparser),
     ('mnist', mnist.fill_subparser),
diff --git a/fuel/downloaders/caltech101_silhouettes.py b/fuel/downloaders/caltech101_silhouettes.py
new file mode 100644
index 0000000..6a6e069
--- /dev/null
+++ b/fuel/downloaders/caltech101_silhouettes.py
@@ -0,0 +1,38 @@
+from fuel.downloaders.base import default_downloader
+
+
+BASE_URL = 'https://people.cs.umass.edu/~marlin/data/'
+FILENAME = 'caltech101_silhouettes_{}_split1.mat'
+
+
+def silhouettes_downloader(size, **kwargs):
+    if size not in (16, 28):
+        raise ValueError("size must be 16 or 28")
+
+    actual_filename = FILENAME.format(size)
+    actual_url = BASE_URL + actual_filename
+    default_downloader(urls=[actual_url],
+                       filenames=[actual_filename], **kwargs)
+
+
+def fill_subparser(subparser):
+    """Sets up a subparser to download the Silhouettes dataset files.
+
+    The following CalTech 101 Silhouette dataset files can be downloaded
+    from Benjamin M. Marlin's website [MARLIN]:
+        `caltech101_silhouettes_16_split1.mat` and
+        `caltech101_silhouettes_28_split1.mat`.
+
+    .. [MARLIN] https://people.cs.umass.edu/~marlin/data.shtml
+
+    Parameters
+    ----------
+    subparser : :class:`argparse.ArgumentParser`
+        Subparser handling the `caltech101_silhouettes` command.
+
+    """
+    subparser.add_argument(
+        "size", type=int, choices=(16, 28),
+        help="height/width of the datapoints")
+    subparser.set_defaults(
+        func=silhouettes_downloader)


Test patch:
diff --git a/tests/test_caltech101_silhouettes.py b/tests/test_caltech101_silhouettes.py
new file mode 100644
index 0000000..95fc055
--- /dev/null
+++ b/tests/test_caltech101_silhouettes.py
@@ -0,0 +1,48 @@
+import numpy
+from numpy.testing import assert_raises
+
+from fuel.datasets import CalTech101Silhouettes
+from tests import skip_if_not_available
+
+
+def test_caltech101_silhouettes16():
+    skip_if_not_available(datasets=['caltech101_silhouettes16.hdf5'])
+    for which_set, size, num_examples in (
+            ('train', 16, 4082), ('valid', 16, 2257), ('test', 16, 2302)):
+        ds = CalTech101Silhouettes(which_sets=[which_set], size=size,
+                                   load_in_memory=False)
+
+        assert ds.num_examples == num_examples
+
+        handle = ds.open()
+        features, targets = ds.get_data(handle, slice(0, 10))
+
+        assert features.shape == (10, 1, size, size)
+        assert targets.shape == (10, 1)
+
+        assert features.dtype == numpy.uint8
+        assert targets.dtype == numpy.uint8
+
+
+def test_caltech101_silhouettes_unkn_size():
+    assert_raises(ValueError, CalTech101Silhouettes,
+                  which_sets=['test'], size=10)
+
+
+def test_caltech101_silhouettes28():
+    skip_if_not_available(datasets=['caltech101_silhouettes28.hdf5'])
+    for which_set, size, num_examples in (
+            ('train', 28, 4100), ('valid', 28, 2264), ('test', 28, 2307)):
+        ds = CalTech101Silhouettes(which_sets=[which_set], size=size,
+                                   load_in_memory=False)
+
+        assert ds.num_examples == num_examples
+
+        handle = ds.open()
+        features, targets = ds.get_data(handle, slice(0, 10))
+
+        assert features.shape == (10, 1, size, size)
+        assert targets.shape == (10, 1)
+
+        assert features.dtype == numpy.uint8
+        assert targets.dtype == numpy.uint8
diff --git a/tests/test_converters.py b/tests/test_converters.py
index 0e94617..67cc843 100644
--- a/tests/test_converters.py
+++ b/tests/test_converters.py
@@ -18,7 +18,9 @@ from six.moves import range, zip, cPickle
 
 from fuel.converters.base import (fill_hdf5_file, check_exists,
                                   MissingInputFiles)
-from fuel.converters import binarized_mnist, cifar10, mnist, cifar100, svhn
+from fuel.converters import (binarized_mnist, caltech101_silhouettes,
+                             cifar10, cifar100, mnist, svhn)
+from fuel.downloaders.caltech101_silhouettes import silhouettes_downloader
 
 if six.PY3:
     getbuffer = memoryview
@@ -382,6 +384,46 @@ class TestCIFAR100(object):
                      ('batch', 'index'))
 
 
+class TestCalTech101Silhouettes(object):
+    def setUp(self):
+        self.tempdir = tempfile.mkdtemp()
+
+    def tearDown(self):
+        shutil.rmtree(self.tempdir)
+
+    def test_download_and_convert(self, size=16):
+        tempdir = self.tempdir
+
+        cwd = os.getcwd()
+        os.chdir(tempdir)
+
+        assert_raises(MissingInputFiles,
+                      caltech101_silhouettes.convert_silhouettes,
+                      size=16, directory=tempdir,
+                      output_directory=tempdir)
+        assert_raises(ValueError, silhouettes_downloader,
+                      size=10, directory=tempdir)
+
+        silhouettes_downloader(size=size, directory=tempdir)
+
+        assert_raises(ValueError,
+                      caltech101_silhouettes.convert_silhouettes,
+                      size=10, directory=tempdir,
+                      output_directory=tempdir)
+
+        caltech101_silhouettes.convert_silhouettes(size=size,
+                                                   directory=tempdir,
+                                                   output_directory=tempdir)
+
+        os.chdir(cwd)
+
+        output_file = "caltech101_silhouettes{}.hdf5".format(size)
+        output_file = os.path.join(tempdir, output_file)
+        with h5py.File(output_file, 'r') as h5:
+            assert h5['features'].shape == (8641, 1, size, size)
+            assert h5['targets'].shape == (8641, 1)
+
+
 class TestSVHN(object):
     def setUp(self):
         numpy.random.seed(9 + 5 + 2015)
diff --git a/tests/test_downloaders.py b/tests/test_downloaders.py
index 06baa84..0907d82 100644
--- a/tests/test_downloaders.py
+++ b/tests/test_downloaders.py
@@ -7,7 +7,8 @@ from functools import wraps
 
 from numpy.testing import assert_equal, assert_raises
 
-from fuel.downloaders import mnist, binarized_mnist, cifar10, cifar100, svhn
+from fuel.downloaders import (binarized_mnist, caltech101_silhouettes,
+                              cifar10, cifar100, mnist, svhn)
 from fuel.downloaders.base import (download, default_downloader,
                                    filename_from_url, NeedURLPrefix,
                                    ensure_directory_exists)
@@ -117,6 +118,16 @@ def test_binarized_mnist():
     assert args.func is default_downloader
 
 
+def test_caltech101_silhouettes():
+    parser = argparse.ArgumentParser()
+    subparsers = parser.add_subparsers()
+    caltech101_silhouettes.fill_subparser(
+        subparsers.add_parser('caltech101_silhouettes'))
+    args = parser.parse_args(['caltech101_silhouettes', '16'])
+    assert_equal(args.size, 16)
+    assert args.func is caltech101_silhouettes.silhouettes_downloader
+
+
 def test_cifar10():
     parser = argparse.ArgumentParser()
     subparsers = parser.add_subparsers()