Add initializers #116

JimClarke5 · 2020-09-20T15:44:31Z

This PR adds initializers to tensorflow-framework. The classes inherit from BaseInitializer which implements Initializer which is also defined as a Functional interface so that an Initializer can be a lambda. Unit Test cases are also provided.

I have also included utils.ShapeUtils. Some methods in ShapeUtils,

public static boolean isCompatibleWith(Shape a, Shape b),
public static Shape reduce(Shape shape, int axis)
public static boolean isUnknownShape(Shape a)

might be considered to be a part of org.tensorflow.ndarray.Shape. The other methods bridge Operands to a org.tensorflow.ndarray.Shape, so probably don't belong in the ndarray module.

This PR is not dependent on any other PR.

Fixed dependencies in pom.xml

…hod. This allows the NAME to be used elsewhere instead of hardcoding the string.

…coding the string. added methods isFloating(), isInteger(), isNUmeric(), isBoolean() and isString()

…ng OP_NAME" to each generated operation.

…EntropyWitLogits() Added tf.nn.sparesSoftmaxCrossEntropyWithLogits() and tf.nn.raw.sparesSoftmaxCrossEntropyWithLogits() Added tf.nn.sigmoidCrossEntropyWithLogits()

…Logits to org.tensorflow.op.nn.raw

…XXXX";

fix javadoc, fix casts

…yWithLogits.java to nn.raw, added new versions of these to NnOps

…maxCrossEntropyWithLogits()

…x JavaDoc. Change from snake case to camel case.

…Java files for some Ops. This also resulted in new generated source that are also committed.

…nerics.

…gmoidCrossEntropyWithLogits.java, and SparseSoftmaxCrossEntropyWithLogits.java under package org.tensorflow.op.nn in

…asy inclusion of a default optimizer. Cleaned up JavaDoc

Craigacp

The seeds should be primitives not boxed primitives that'll let you elide the conditional logic. Either that or document that if the seed is null it defaults to zero (but I'd prefer them to be primitives). Other than that and a missing bit of Javadoc in VarianceScaling this looks good and is ready to be merged.

Craigacp · 2020-09-30T14:16:03Z

tensorflow-framework/src/main/java/org/tensorflow/framework/initializers/Orthogonal.java

+    for (; i < dimsShape.numDimensions() - 1; i++) num_rows *= dimsShape.size(i);
+    long num_cols = dimsShape.size(i);
+    Shape flat_shape = Shape.of(Math.max(num_rows, num_cols), Math.min(num_rows, num_cols));
+    long lseed = this.seed == null ? 0L : this.seed;


This isn't necessary if we make seed be a long not a Long. I think we should prevent people from passing nulls to the constructor.

Craigacp

LGTM, thanks Jim.

karllessard · 2020-10-02T20:00:44Z

tensorflow-framework/src/main/java/org/tensorflow/framework/initializers/Identity.java

+    }
+    boolean isSquare = shape.size(0) == shape.size(1);
+    long diag_size = Math.min(shape.size(0), shape.size(1));
+    Shape diagShape = Shape.of(diag_size);


There is still a few variables like this in the PR using snake_case instead of camelCase.

karllessard

I think we all agree that in general, this PR is ready to be merged. Up to you @JimClarke5 to update now the small details we've mentioned or do it later (I like though the idea of moving isCompatibleWith to the ndarray library).

karllessard · 2020-10-02T20:12:37Z

Another thing, I have some concerns about the naming of some classes, like Constant, which can conflict with the Constant class in the core API, forcing the user to use their canonical names in case both types are present in some files.

I understand though that if we prefix/suffix this class, we should do it for all of our initializers as well (and what about our current optimizers?). So maybe we should just find another name for these?

Note that we are never protected from conflicts with the ops classes since most of them are generated. For example one day, a Ones kernel might appear as well...

JimClarke5 · 2020-10-02T20:49:34Z

I have fixed the snake_case issues.

The names are the same ones used in Keras. IMO, most users will use tf.constant() and not the ops Constant class directly.

Craigacp · 2020-10-02T20:55:55Z

I have fixed the snake_case issues.

The names are the same ones used in Keras. IMO, most users will use tf.constant() and not the ops Constant class directly.

Yeah, but if they assign tf.constant() to a variable they'll need to bring the Constant type into scope unless they are using Java 10+ and use var. So it'll conflict.

JimClarke5 · 2020-10-02T20:59:46Z

It's besides the point but Shape already conflicts with org/tensorflow.op.core.Shape and org.tensorflow.ndarray.Shape.

If you want, I can rename them all to xxxxxInitialzier.

JimClarke5 · 2020-10-03T12:00:11Z

I have moved isCompatibleWith to ndarray Shape, and added test for it in ShapeTest.

karllessard · 2020-10-03T23:00:05Z

I have fixed the snake_case issues.
The names are the same ones used in Keras. IMO, most users will use tf.constant() and not the ops Constant class directly.

Yeah, but if they assign tf.constant() to a variable they'll need to bring the Constant type into scope unless they are using Java 10+ and use var. So it'll conflict.

Exactly the case I had in mind as well. They can just keep a reference to Operand and it will be fine but I personally like to preserve the original type of the ops as I add it to the graph. But like you said, in Java 10+ and Kotlin it will be less frequent. So let’s keep the current naming strategy for now and review it later if needed.

karllessard · 2020-10-03T23:04:34Z

So this PR is good to go, thanks @JimClarke5 ! Please review the conflict raised in Shape and I’ll merge it after the release of 0.2.0 (which I plan to do this weekend)

JimClarke5 · 2020-10-04T19:13:23Z

The conflict in Shape had to do with JavaDoc indenting. As I am using the Google Java formatter in InteliliJ, I resolved he conflict with my version.

Did we want to rename Constant to ConstantInitializer, Zeros to ZerosInitializer and Ones to OnesInitializer ?
I noticed that in TF Python, 2.3.1, there are now initializer methods defined in init_ops_v2.py directly under the tensorflow/python/ops directory (outside of Keras), called tf.constant_initializer, presumably to avoid the conflict with tf.constant, similar to our predicament. They also have tf.ones_initializer, tf.zeros_initializer, among others. They use the same Class names as we currently have, but the method aliases are all in the form tf.xxxx_initializer.

karllessard · 2020-10-07T00:29:14Z

If we do it for the initializers, we should do it for the optimizers as well for consistency. That is why I think it can be out of scope of this PR to make that change and we can do it later, wdyt?

JimClarke5 · 2020-10-07T00:50:02Z

I vote we leave it as is for now. We still have losses and metrics to go, so by then we can get a holistic view how to handle this issue.

karllessard · 2020-10-08T01:21:19Z

@JimClarke5 , I’m sorry to tell you this but it looks like your PR has new conflicts since I’ve updated the master branch for the next development iteration. Can you please fix them so we can merge it?

karllessard · 2020-10-08T01:23:03Z

Ok that’s fine, it just had trouble to rebase but a simple merge did the trick, thanks for your great work!

JimClarke5 · 2020-10-08T01:32:33Z

So I don’t have to fix anything, right?

karllessard · 2020-10-08T17:17:40Z

Nope, it’s already merged! I had to do a few quick fixes in the javadoc though to be able to deploy this new snapshot, we should enable these lint checks locally so a developer can catch the errors upfront, there is a story about it: #7

JimClarke5 added 30 commits July 28, 2020 16:26

Initial checkin of Keras Optimzers and helper classes.

ef0ce67

Fixed dependencies in pom.xml

Added static final NAME to replace hardcoded String in the create met…

9c113a7

…hod. This allows the NAME to be used elsewhere instead of hardcoding the string.

Changed of method to use the DataType NAME attribute rather than hard…

824d487

…coding the string. added methods isFloating(), isInteger(), isNUmeric(), isBoolean() and isString()

Added method WriteFieldWithInitializer to output a "final static Stri…

07a83a5

…ng OP_NAME" to each generated operation.

Added tf.nn.softmaxCrossEntropyWitLogits() and tf.nn.raw.softmaxCross…

3d26831

…EntropyWitLogits() Added tf.nn.sparesSoftmaxCrossEntropyWithLogits() and tf.nn.raw.sparesSoftmaxCrossEntropyWithLogits() Added tf.nn.sigmoidCrossEntropyWithLogits()

Moved SoftmaxCrossEntropyWithLogits and SparseSoftmaxCrossEntropyWith…

11cda5f

…Logits to org.tensorflow.op.nn.raw

Generated classes now have public static final String OP_NAME = "XXXX…

9c7dfaa

…XXXX";

Generated classes now have public static final String OP_NAME = "XXXX…

84f49db

…XXXX";

fix dependencies for other Tensorflow Java modules

208b84a

formatting fix

3913161

Fix ctors with name to properly pass the name to the the super ctor.

b5a7c0f

change asserts to IllegalArgumentException

fcba0a5

fix javadoc, fix casts

change asserts to IllegalArgumentException

960cfc3

Moved back to tests

d37298a

Moved SoftmaxCrossEntropyWithLogits.java and SparseSoftmaxCrossEntrop…

c68812c

…yWithLogits.java to nn.raw, added new versions of these to NnOps

Deleted files that are not necessary yet

6b8eb26

Added nn.raw group for softmaxCrossEntropyWithLogits() and sparseSoft…

6515c24

…maxCrossEntropyWithLogits()

Added nn.raw group for softmaxCrossEntropyWithLogits() and sparseSoft…

76d0fe5

…maxCrossEntropyWithLogits()

Merge branch 'master' into master

d2201df

Refactor NN into individual operations under org.tensorflow.op.nn. Fi…

ab379d1

…x JavaDoc. Change from snake case to camel case.

Refactor NN into individual operations under org.tensorflow.op.nn. Fi…

889d67e

…x JavaDoc. Change from snake case to camel case.

Reformatted code

515b799

Added sub scope

5a9fe37

Miscellaneous fixes based on review comments.

8d21dd7

Fixed op_generator.cc to remove a spurious new line in the generated …

4c3cc78

…Java files for some Ops. This also resulted in new generated source that are also committed.

Changed back to non-generic Operand until we resolve how to handle ge…

44f530f

…nerics.

Regenerated due to creation of SoftmaxCrossEntropyWithLogits.java, Si…

b8d3ac2

…gmoidCrossEntropyWithLogits.java, and SparseSoftmaxCrossEntropyWithLogits.java under package org.tensorflow.op.nn in

change snake case to camel case. format code

c32fc5b

clean upd warning, format code

171cd2f

Added Adamax, Ftrl, and Nadam Optimizers. Added Optimizers enum for e…

e9c3134

…asy inclusion of a default optimizer. Cleaned up JavaDoc

Change UNTRUNCATED_NORMAL to NORMAL

fb52dd4

Craigacp requested changes Sep 30, 2020

View reviewed changes

JimClarke5 added 2 commits September 30, 2020 17:04

Changed Long seed to long seed.

e328cbf

Remove snake case

80f2fb0

Craigacp previously approved these changes Oct 2, 2020

View reviewed changes

karllessard reviewed Oct 2, 2020

View reviewed changes

JimClarke5 added 2 commits October 2, 2020 16:31

Change snake case to camel case

b0b747e

Change snake case to camel case

505d0d6

JimClarke5 dismissed Craigacp’s stale review via 505d0d6 October 2, 2020 20:50

JimClarke5 added 2 commits October 2, 2020 17:25

Moved isCompatibleWith to Shape

c4a7bfb

Moved isCompatibleWith to Shape

9229789

Merge branch 'master' into Initializers1

f0934ea

karllessard approved these changes Oct 8, 2020

View reviewed changes

karllessard merged commit a85bcfb into tensorflow:master Oct 8, 2020

JimClarke5 deleted the Initializers1 branch October 8, 2020 22:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initializers #116

Add initializers #116

JimClarke5 commented Sep 20, 2020

Craigacp left a comment

Craigacp Sep 30, 2020

Craigacp left a comment

karllessard Oct 2, 2020 •

edited

Loading

karllessard left a comment

karllessard commented Oct 2, 2020 •

edited

Loading

JimClarke5 commented Oct 2, 2020

Craigacp commented Oct 2, 2020

JimClarke5 commented Oct 2, 2020 •

edited

Loading

JimClarke5 commented Oct 3, 2020

karllessard commented Oct 3, 2020

karllessard commented Oct 3, 2020 •

edited

Loading

JimClarke5 commented Oct 4, 2020

karllessard commented Oct 7, 2020

JimClarke5 commented Oct 7, 2020

karllessard commented Oct 8, 2020

karllessard commented Oct 8, 2020

JimClarke5 commented Oct 8, 2020 •

edited

Loading

karllessard commented Oct 8, 2020

Add initializers #116

Add initializers #116

Conversation

JimClarke5 commented Sep 20, 2020

Craigacp left a comment

Choose a reason for hiding this comment

Craigacp Sep 30, 2020

Choose a reason for hiding this comment

Craigacp left a comment

Choose a reason for hiding this comment

karllessard Oct 2, 2020 • edited Loading

Choose a reason for hiding this comment

karllessard left a comment

Choose a reason for hiding this comment

karllessard commented Oct 2, 2020 • edited Loading

JimClarke5 commented Oct 2, 2020

Craigacp commented Oct 2, 2020

JimClarke5 commented Oct 2, 2020 • edited Loading

JimClarke5 commented Oct 3, 2020

karllessard commented Oct 3, 2020

karllessard commented Oct 3, 2020 • edited Loading

JimClarke5 commented Oct 4, 2020

karllessard commented Oct 7, 2020

JimClarke5 commented Oct 7, 2020

karllessard commented Oct 8, 2020

karllessard commented Oct 8, 2020

JimClarke5 commented Oct 8, 2020 • edited Loading

karllessard commented Oct 8, 2020

karllessard Oct 2, 2020 •

edited

Loading

karllessard commented Oct 2, 2020 •

edited

Loading

JimClarke5 commented Oct 2, 2020 •

edited

Loading

karllessard commented Oct 3, 2020 •

edited

Loading

JimClarke5 commented Oct 8, 2020 •

edited

Loading