VGG'11 model on FashionMNIST dataset by zaleslaw · Pull Request #16 · tensorflow/java-models

zaleslaw · 2020-06-26T14:59:47Z

Related to the issue #11

zaleslaw · 2020-06-26T15:00:43Z

@Craigacp and @karllessard please have a look

Craigacp

LGTM, thanks.

Craigacp · 2020-06-26T15:41:32Z

...w-examples/src/main/java/org/tensorflow/model/examples/fashionmnist/FashionMnistDataset.java

+        this.imageSize = trainingImages.get(0).shape().size();
+    }
+
+    private static ByteNdArray readArchive(String archiveName) throws IOException {


Can we use the mnist loader here?

Yes, but I tried don't touch the MNISTDataset code here. The best idea - to refactor (convert path to archives to paramters in constructor, for example). In this way, it will have the common loader parametrized by path

Craigacp · 2020-06-26T15:41:58Z

tensorflow-examples/src/main/java/org/tensorflow/model/examples/fashionmnist/VGGModel.java

+import org.tensorflow.framework.optimizers.Optimizer;
+import org.tensorflow.model.examples.mnist.data.ImageBatch;
+import org.tensorflow.op.Ops;
+import org.tensorflow.op.core.*;


I'd prefer no star imports in the example code too, @karllessard what do you think?

Craigacp · 2020-06-26T15:47:04Z

...flow-examples/src/main/java/org/tensorflow/model/examples/mnist/data/ImageBatchIterator.java

 import org.tensorflow.tools.ndarray.index.Index;

-class ImageBatchIterator implements Iterator<ImageBatch> {
+public class ImageBatchIterator implements Iterator<ImageBatch> {


These should probably have a little bit of documentation if they are public.

zaleslaw · 2020-07-13T13:30:03Z

@Craigacp @karllessard I fixed comments in this repository codestyle.
Also, I suggested a few minor improvements in package structure: I'm going to add more examples for diferent architectures (but it could be based on the same datasets) and as a result I suggest the model-architecture centric package naming (not dataset as now).

What do you think?

karllessard · 2020-07-22T02:45:37Z

Hey @zaleslaw , sorry for the late reply, looks good on my side, I'm ok with this new organization. The only comment is that the datasets package is at the same level as the examples, so it is a bit misleading as one might expect to see examples of Dataset usage there, while there are just common utilities for the other examples. Should we rename it common then?

I did not went in deep analysis of your VGG model though but I trust you there ;) Wdyt @Craigacp ?

zaleslaw · 2020-07-22T08:25:37Z

@karllessard I'll fix the package structure according your comment.
About VGG model - it's close to CNN example (I tried to keep the structure and naming in one style), but added some high-level functions for layer building, it's not Keras yet, but close to the Sequential model logic

Craigacp

Are we ok to redistribute MNIST and FashionMNIST?

Craigacp · 2020-07-22T17:06:04Z

...orflow-examples/src/main/java/org/tensorflow/model/examples/cnn/vgg/VGG11OnFashionMNIST.java

@@ -0,0 +1,50 @@
+/*
+ * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.


This doesn't want an Oracle copyright at the top.

Wow, thanks!

Craigacp · 2020-07-22T19:00:45Z

...orflow-examples/src/main/java/org/tensorflow/model/examples/datasets/mnist/MnistDataset.java

-      ByteNdArray trainingLabels = readArchive(TRAINING_LABELS_ARCHIVE);
-      ByteNdArray testImages = readArchive(TEST_IMAGES_ARCHIVE);
-      ByteNdArray testLabels = readArchive(TEST_LABELS_ARCHIVE);
+      ByteNdArray trainingImages = readArchive(trainingImagesArchive);


Maybe there should be two methods here, one for MNIST and one for FashionMNIST? Then we don't have to carry around the string constants in all the classes that use them. This method can remain if people want to load in the extra MNIST test data from last years NeurIPS by supplying their own downloaded version.

Maybe, but I've copied an existing approach, from my perspective for examples the best way to have them as a resources.

karllessard · 2020-07-22T20:06:55Z

Are we ok to redistribute MNIST and FashionMNIST?

I think that is a fair question. When I've dropped the MNIST dataset in the resource folder at first, it was kind of a temporary solution until we write a simple HTTP client to download it instead. Now by adding this new dataset, we are getting closer to crystallize this hack as something permanent, which I would prefer we avoid before the project grows too much...

So if you are ready to tackle this @zaleslaw , it would be great to change the data loader to use an HTTP client instead and remove the datasets from the resource folder. Wdyt?

zaleslaw · 2020-07-23T15:09:23Z

@karllessard I could switch and return to MNIST to run the VGG model and remove Fashion MNIST but I'm not ready now to write http loader. My point of view - that resources is the best place to keep small datasets for examples, it's not a hack, it's a feature:) if datasets are small like MNIST and FashionMNIST.

Maybe I'm wrong about datasets keeping, and we could separate it on two parts - merge this and create a ticket for HTTP loader?

What do you think, guys?

Craigacp · 2020-07-23T16:17:55Z

I'd be happy to redistribute them if we have the rights to do so, but it's a legal question not a convenience one.

zaleslaw · 2020-07-28T12:04:43Z

@karllessard I've repaired the license in code files and checked license for FashionMNIST dataset
Also I added a Readme file with the link to the dataset

I suggest to merge PR and create the separate ticket for HTTP client to obtain datasets from the web

zaleslaw added 2 commits June 26, 2020 17:56

Added VGG example on Fashion Mnist Dataset

d7d1fcd

Added licenses

108cfbc

googlebot added the cla: yes label Jun 26, 2020

Craigacp reviewed Jun 26, 2020

View reviewed changes

Fixed notes and renamed the packages

bbc6b50

Fixed the package name

1c7369a

Craigacp reviewed Jul 22, 2020

View reviewed changes

Fixed the licenses and added citation for FashionMNIST

ea7a70c

karllessard approved these changes Jul 28, 2020

View reviewed changes

karllessard merged commit 1177e2c into tensorflow:master Jul 28, 2020

		@@ -0,0 +1,50 @@
		/*
		* Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.

Conversation

zaleslaw commented Jun 26, 2020

Uh oh!

zaleslaw commented Jun 26, 2020

Uh oh!

Craigacp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zaleslaw commented Jul 13, 2020

Uh oh!

karllessard commented Jul 22, 2020

Uh oh!

zaleslaw commented Jul 22, 2020

Uh oh!

Craigacp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Craigacp Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karllessard commented Jul 22, 2020

Uh oh!

zaleslaw commented Jul 23, 2020

Uh oh!

Craigacp commented Jul 23, 2020

Uh oh!

zaleslaw commented Jul 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Craigacp Jul 22, 2020 •

edited

Loading