mnist and mnistm dataloaders #20

arikanev · 2017-12-25T03:54:40Z

Mnist segmentations are useful for quick training when testing models

pep8speaks · 2017-12-25T03:54:42Z

Hello @arikanev! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on January 15, 2018 at 16:55 Hours UTC

fvisin

Thank you for your PR!
There are a few things to fix, but LGTM overall.
Once you fix these it can be merged! Thanks

fvisin · 2017-12-29T10:19:35Z

dataset_loaders/images/mnist.py

+import errno
+from PIL import Image
+from dataset_loaders.parallel_loader import ThreadedDataset
+from timeit import default_timer as timer


Can you reorder the imports so that they are grouped according to https://www.python.org/dev/peps/pep-0008/#imports and each chunk is alphabetically ordered?

fvisin · 2017-12-29T10:20:36Z

dataset_loaders/images/mnist.py

+class MnistDataset(ThreadedDataset):
+    """The mnist handwritten digit dataset
+
+    The dataset should be downloaded from [1] into the `shared_path`


Please replace [1] with [MNIST]_.

fvisin · 2017-12-29T10:20:50Z

dataset_loaders/images/mnist.py

+
+     References
+    ----------
+    [1] Mnist dataset pickle file:


Please replace [1] with [MNIST]

fvisin · 2017-12-29T10:21:38Z

dataset_loaders/images/mnist.py

+     References
+    ----------
+    [1] Mnist dataset pickle file:
+        your_username_here@elisa1.iro.umontreal.ca:/data/lisa/data/mnist/mnist_seg/


Please don't make this MILA specific. Replace with the MNIST website or the download link.

fvisin · 2017-12-29T10:22:16Z

dataset_loaders/images/mnist.py

+    # optional arguments
+    data_shape = (28, 28, 3)
+    mean = [0, 0, 0]
+    std = [1, 1, 1]


No need to specify mean and std unless you actually compute them. Please either compute them or remove it.

fvisin · 2017-12-29T10:24:26Z

dataset_loaders/images/mnist.py

+    max_files = 50000
+
+    GTclasses = range(2)
+    mapping_type = 'mnist'


This is never used. Please remove.

fvisin · 2017-12-29T10:27:27Z

dataset_loaders/images/mnist.py

+    non_void_nclasses = 2
+    _void_labels = []
+    GTclasses = range(2)
+    # GTclasses = GTclasses + [-1]


Please remove the commented line

fvisin · 2017-12-29T10:34:03Z

dataset_loaders/images/mnist.py

+        your_username_here@elisa1.iro.umontreal.ca:/data/lisa/data/mnist/mnist_seg/
+
+    """
+


Remove the empty line please

fvisin · 2017-12-29T10:34:12Z

dataset_loaders/images/mnist.py

+        it also creates/copies the dataset in self.path if not already there
+          mnist data is in 3 directories train, test, valid)
+        """
+


Remove the empty line please

fvisin · 2017-12-29T16:51:14Z

dataset_loaders/images/mnistm.py

+from dataset_loaders.parallel_loader import ThreadedDataset
+
+
+class MnistMDataset(ThreadedDataset):


Please inherit from MnistDataset and modify only the relevant parts.

arikanev · 2018-01-09T23:13:46Z

Implemented changes as requested, let me know if I should modify anything

fvisin

Thank you for your new commit. There is still a lot of repeated code in the MnistmDataset class, please remove it. I also noticed a couple of new things that should be fixed.

When you are done, please also add the new datasets to the __init__.py in the root (follow what was done for the other datasets).

Thank you!

fvisin · 2018-01-11T20:27:32Z

dataset_loaders/images/mnist.py


-    The dataset should be downloaded from [1] into the `shared_path`
+    The dataset should be downloaded from [MNIST] into the `shared_path`


"[MNIST]" should be "[MNIST]_"

fvisin · 2018-01-12T11:55:39Z

dataset_loaders/images/mnistm.py

+class MnistMDataset(MnistDataset):
+    """The mnist-m handwritten digit dataset.
+
+    The dataset should be downloaded from [1]


Fix the link as done for the other file. Also please add a small description of the dataset.

Will do. I want to add the download to mnistm via my website akanev.com. Should I wait to push changes until I link it, or should I push a change to just the mnistm download link later on? Or neither and link via google drive.

Will push changes now and then push that later

Is there any advantage to download through your website rather than the original download link? I think it's best to link to an established website to avoid to potentially generate confusion in the users and to make sure the link is up to date in case the original authors notice some problem with the data.

There is no really established website for mnistm. I believe Yaroslav, (a MILA phd student), created it for previous research. There is one link to the old mnistm data via google drive, which I think could look sketchy. The new mnistm that I generated using a script is not available via the web right now, which is why I wanted to upload it.

Also, theres some errors in the code when inheriting mnistm from mnist that I need to work out. So I will just push mnist for now, and fix the mnistm issues in the meantime.

fvisin · 2018-01-12T11:56:23Z

dataset_loaders/images/mnist.py

+        }
+
+    _cmap = {
+        0: (0),           # background


cmap should return RGB triples. Please fix this.

Ok. To confirm: (0, 0, 0)?

fvisin · 2018-01-12T12:10:24Z

dataset_loaders/images/mnist.py

+    name = 'mnist'
+
+    # optional arguments
+    data_shape = (28, 28, 3)


I believe MNIST data_shape should be (28, 28). Can you verify this?

Ah yes sorry.

No problem :)

fvisin · 2018-01-12T12:12:04Z

dataset_loaders/images/mnistm.py

+    # optional arguments
+    data_shape = (28, 28, 3)
+
+    GTclasses = range(2)


This isn't necessary, as is already defined in MnistDataset. The same goes for all the other properties that are not different between the two.

fvisin · 2018-01-12T12:14:08Z

dataset_loaders/images/mnistm.py

+
+    _filenames = None
+
+    def __init__(self, which_set='train', *args, **kwargs):


This method is the same as the one in MnistDataset. There is no need to redefine it.
The same goes with all the other methods: remove the parts that do not change.

arikanev · 2018-01-15T16:56:25Z

Removed mnistm.py, mnist.py should have all of the necessary changes you requested.

mnist and mnistm dataloaders

9169ac1

pep8 compatibility changes

81b51de

fvisin reviewed Dec 29, 2017

View reviewed changes

fvisin added the changes requested label Dec 29, 2017

implemented requested changes

8cf4966

fvisin suggested changes Jan 12, 2018

View reviewed changes

temp removal of mnistm.py

3292708

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mnist and mnistm dataloaders #20

mnist and mnistm dataloaders #20

arikanev commented Dec 25, 2017 •

edited

Loading

pep8speaks commented Dec 25, 2017 •

edited

Loading

fvisin left a comment

fvisin Dec 29, 2017

fvisin Dec 29, 2017

fvisin Dec 29, 2017

fvisin Dec 29, 2017

fvisin Dec 29, 2017

fvisin Dec 29, 2017

fvisin Dec 29, 2017

fvisin Dec 29, 2017

fvisin Dec 29, 2017

fvisin Dec 29, 2017

arikanev commented Jan 9, 2018

fvisin left a comment

fvisin Jan 11, 2018

arikanev Jan 12, 2018

fvisin Jan 12, 2018

arikanev Jan 12, 2018 •

edited

Loading

arikanev Jan 12, 2018 •

edited

Loading

fvisin Jan 15, 2018

arikanev Jan 15, 2018

arikanev Jan 15, 2018

fvisin Jan 12, 2018

arikanev Jan 12, 2018

fvisin Jan 12, 2018

fvisin Jan 12, 2018

arikanev Jan 12, 2018 •

edited

Loading

fvisin Jan 12, 2018

arikanev Jan 12, 2018

fvisin Jan 12, 2018

arikanev Jan 12, 2018

fvisin Jan 12, 2018

arikanev Jan 12, 2018

arikanev commented Jan 15, 2018

		your_username_here@elisa1.iro.umontreal.ca:/data/lisa/data/mnist/mnist_seg/

		"""

		from dataset_loaders.parallel_loader import ThreadedDataset


		class MnistMDataset(ThreadedDataset):


		The dataset should be downloaded from [1] into the `shared_path`
		The dataset should be downloaded from [MNIST] into the `shared_path`


		_filenames = None

		def __init__(self, which_set='train', args, *kwargs):

mnist and mnistm dataloaders #20

Are you sure you want to change the base?

mnist and mnistm dataloaders #20

Conversation

arikanev commented Dec 25, 2017 • edited Loading

pep8speaks commented Dec 25, 2017 • edited Loading

Comment last updated on January 15, 2018 at 16:55 Hours UTC

fvisin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arikanev commented Jan 9, 2018

fvisin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arikanev Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

arikanev Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arikanev Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arikanev commented Jan 15, 2018

arikanev commented Dec 25, 2017 •

edited

Loading

pep8speaks commented Dec 25, 2017 •

edited

Loading

arikanev Jan 12, 2018 •

edited

Loading

arikanev Jan 12, 2018 •

edited

Loading

arikanev Jan 12, 2018 •

edited

Loading