Generic cleanup rest of framework, activations and initializers #231

JimClarke5 · 2021-03-03T17:27:47Z

This PR cleans up the generics in activations and initializers.
Basically the generic was removed from the class declaration and moved to the call() method.

For example, Activation:

public abstract class Activation {
....
    public abstract <T extends TNumber> Operand<T> call(Operand<T> input);
}

and Initializer:

public interface Initializer {
    <T extends TType> Operand<T> call(Operand<TInt64> dims, Class<T> type);
}

Sync with master tensorflow on upstream

Merge main branch to local branch

Update after losses merge

Fix Javadoc errors (tensorflow#152)

pull type def

merge

Metrics Phase 1 (tensorflow#180)

Pull latest tensorflow master

Merge with latest

…have generic.

…or the other xxxxOps classes changes.

Resync with origin/master

…ric_cleanup_rest_of_fmwork

karllessard

Thanks @JimClarke5 , I've left a few minor comments on this.

karllessard · 2021-03-04T01:17:47Z

tensorflow-framework/src/main/java/org/tensorflow/framework/activations/Activation.java

  }

  /**
   * Gets the calculation operation for the activation.
   *
   * @param input the input tensor
   * @return The operand for the activation
+   * @param <T> the data type of the input and result


can you reorder these tags so that the @param are all before @return? I think the generic parameter should be the first one, I'm not sure if there is a standard for this. Or, if you prefer, I would also be totally comfortable not documenting at all the generic parameter of these method, which is quite explicit. Your choice.

This comment applies to other places in that PR.

I did find this document from Oracle, How to Write Doc Comments for the Javadoc Tool.
@param should be before @return, but it doesn't mention anything about ordering the generic within the params.
It just says Multiple @param tags should be listed in argument-declaration order. This makes it easier to visually match the list to the declaration.

I will fix by moving the @param<T> to before the @return

Javadoc will warn on any missing parameters, generic or otherwise, so I think it's better to have all parameters documented.

karllessard · 2021-03-04T01:47:09Z

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

+  public <T extends TType> Operand<T> call(Operand<TInt64> dims, Class<T> type) {
+    if (!TNumber.class.isAssignableFrom(type)) {
+      throw new IllegalArgumentException("Tensor type must be numeric: " + type.getSimpleName());
+    }


Just double-checking if that's correct, the initial generic at the class level was bound to TFloating while now it is TNumber.

Same thing for a few other initializers below.

Some of the classes were TFloating, some were TNumber or TType for Initializer. If I change the overloaded method to TFloating, the compiler would complain about not overloading the abstract method. I am open to suggestions. Right now I throw IllegalArgumentException which is not optimal.

I have changed to the generic on the base class as @karllessard suggested. This eliminates this issue for the most part.
There is still a runtime check for Zeros and Ones to check if TNumber or TBool.

(See my question elsewhere about adding a superclass TNumberOrBool.)

tensorflow-framework/src/main/java/org/tensorflow/framework/activations/Exponential.java

Craigacp · 2021-03-04T14:52:27Z

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

    if (!TNumber.class.isAssignableFrom(type) && type != TBool.class) {
-      throw new IllegalArgumentException("Tensor type must be numeric or boolean: " + type.getSimpleName());
+      throw new IllegalArgumentException(


Shouldn't this enforce that the calling type is the same as valueType?

It's casting to type. Isn't that good enough?

Well if you've constructed it with a double but call it with TInt32 that sounds like a programmer error to me and so we might want to flag that with a runtime exception.

The other option is to change the value to an Operand, and use that type.

I have changed the value to Operand, the only constraint is that this must be a scalar (added throws IllegalArgumentException in CTOR).

The TF Python Constant has the constraint that the value must be "castable" to the type defined in the call method. So the way I have it matches TF Python. Do we want to create a method in NDArray to detect uncastable pairs?
I think the only noncastable operation is to/from TString.
(Throws org.tensorflow.exceptions.TFUnimplementedException: Cast string to int32 is not supported)

Also, TF Python supports a Constant for TString, but I cannot figure out how to create an Operand from a String[][] with tf.constant(), so the Unit Test case does:

Shape shape = Shape.of(2, 2); String[][] expected = { {"Java Test", "Java Test"}, {"Java Test", "Java Test"}, }; // There is no tf.constant(String[][]). Operand<TString> expectedOp = org.tensorflow.op.core.Constant.tensorOf(tf.scope(), expected); Constant instance = new Constant(tf, tf.constant("Java Test")); Operand<TInt32> result = instance.call(tf.constant(shape), TInt32.class);

I think that's probably ok. It seems unlikely that we'll need to initialise something with a String rather than just define it elsewhere.

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

Craigacp · 2021-03-04T14:58:24Z

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

   */
-  Operand<T> call(Operand<TInt64> dims, Class<T> type);
+  <T extends TType> Operand<T> call(Operand<TInt64> dims, Class<T> type);


Why don't we make this TNumber instead? Seems like that would reduce a lot of issues, and only lose the boolean case (which I'm unclear how much is used).

We could do that and have the calling program cast it to TBool if desired. It only effects Constant and Ones.

I think I would be fine with that. @karllessard @deansher opinions?

Makes sense to me.

Going a little further afield: could we / should we make TBool a subtype of TNumber? When we discuss constraining a tensor type to TNumber, we often end up with "yeah, but TBool".

How about "NotTString" :-)?

The earliest versions of Tribuo had BNumber which was a boolean box that implemented java.lang.Number, but we refactored that out because it was more trouble than it was worth. As such I don't think it's the best idea, especially as TNumber lines up fairly well with things you'd put java.lang.Number on in Java.

Is anyone besides me tempted to add a superclass of TNumber and TBool titled (perhaps, in a burst of creativity) TNumberOrBool?

I think that might be nicer for us, but do we want users to have TNumberOrBool in their code? Because it looks a little messy.

tensorflow-framework/src/test/java/org/tensorflow/framework/activations/ExponentialTest.java

…thod to <U extends T>. Changed all subclasses to match these signatures.

Craigacp

Looks good. I've only got a few things that need discussion or changing, but this is a lot nicer than the previous version and I think it's good to go now.

Craigacp · 2021-03-09T17:02:55Z

tensorflow-framework/src/main/java/org/tensorflow/framework/activations/GeLU.java

+                  tf.math.tanh(
+                      tf.math.mul(
+                          // sqrt(2.0 / PI)
+                          cast(tf, tf.constant(0.7978845608028654), input.type()),


Why isn't this one pulled out like the others?

It was mainly for debugging and keeping the parts of the equation manageable. I will change this one and add one for the constant "three".

BTW: It would be nice if we could pass a type to tf.constant, something liketf.constant(3, input.dtype())to return the correct type.

Sounds like a good extension to have. That should be fairly straightforward.

Craigacp · 2021-03-09T17:03:48Z

tensorflow-framework/src/main/java/org/tensorflow/framework/activations/GeLU.java

+        features / math_ops.cast(1.4142135623730951, features.dtype)))
+       */
+      return tf.math.mul(
+          cast(tf, tf.constant(0.5), input.type()),


Maybe hoist this and the one below out of the if statement and use local variables?

Craigacp · 2021-03-09T17:07:32Z

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

    if (!TNumber.class.isAssignableFrom(type) && type != TBool.class) {
      throw new IllegalArgumentException(
          "Tensor type must be numeric or boolean: " + type.getSimpleName());
    }
-    return tf.fill(dims, tf.dtypes.cast(tf.constant(1.0), type));
+
+    return cast(tf, tf.fill(dims, tf.constant(1)), type);


This switched the order of the fill and the cast. Is there a reason to prefer one way over the other?

I am not sure if it make a difference, but might be faster to fill with the right type first. I will fix.

Craigacp · 2021-03-09T17:09:00Z

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

-        tf.math.mul(distOp, tf.dtypes.cast(tf.constant(this.stddev), distOp.type()));
-    return cast(tf, tf.math.add(op, tf.dtypes.cast(tf.constant(mean), distOp.type())), type);
+    Operand<U> distOp = tf.random.statelessRandomNormal(dims, tf.constant(seeds), type);
+    Operand<U> op = tf.math.mul(distOp, cast(tf, tf.constant(this.stddev), type));


I missed this initially, but is there a reason we don't check that the standard deviation is positive in the constructor?

Craigacp · 2021-03-09T17:09:51Z

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

-    @SuppressWarnings("unchecked")
-    Class<TNumber> nType = (Class<TNumber>) type;
-    Operand<TNumber> distOp;
+  public <U extends TNumber> Operand<U> call(Operand<TInt64> dims, Class<U> type) {


Similar to RandomNormal is there a reason we don't check that the minVal is less than the maxVal on construction?

Craigacp · 2021-03-09T17:11:11Z

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

-        type);
+    Operand<U> distOp = tf.random.statelessTruncatedNormal(dims, tf.constant(seeds), type);
+    return tf.math.add(
+        tf.math.mul(distOp, cast(tf, tf.constant(stddev), distOp.type())),


The code shape here is different to RandomNormal. Can they be the same but only call a different tf.random function?

Craigacp · 2021-03-09T17:14:44Z

tensorflow-framework/src/test/java/org/tensorflow/framework/activations/GeLUTest.java

+  @Test
+  public void testCallFloat() {
+    float[][] input = {
+      {0.22805803f, 0.60407318f, 0.91519962f, 0.35643331f, 0.28702669f},


Can we test some negative values here? Specifically in the region -2,0 where it's negative, and something more negative like -10 where it should be approximately zero.

added additional test case for invalid conversion of TString to TInt32

JimClarke5 added 13 commits October 8, 2020 13:19

Merge pull request #3 from tensorflow/master

c57a2e7

Sync with master tensorflow on upstream

Merge pull request #4 from tensorflow/master

09fc07e

Merge main branch to local branch

Merge pull request #5 from tensorflow/master

a99dcb4

Update after losses merge

Merge pull request #6 from tensorflow/master

ba294ea

Fix Javadoc errors (tensorflow#152)

Merge pull request #7 from tensorflow/master

04f419a

pull type def

Merge pull request #8 from tensorflow/master

02e7ebf

merge

Merge pull request #9 from tensorflow/master

e0c9ed8

Metrics Phase 1 (tensorflow#180)

Merge pull request #10 from tensorflow/master

5b0374b

Pull latest tensorflow master

Merge pull request #11 from tensorflow/master

e038bbd

Merge with latest

Clean up generics, remove generics from class and fix call method to …

28a34dd

…have generic.

resynch with master, for some reason when I build on mac, the order f…

309b834

…or the other xxxxOps classes changes.

Merge pull request #13 from tensorflow/master

def3051

Resync with origin/master

Merge branch 'master' of https://github.com/JimClarke5/java into Gene…

3a9ae37

…ric_cleanup_rest_of_fmwork

karllessard requested changes Mar 4, 2021

View reviewed changes

Craigacp reviewed Mar 4, 2021

View reviewed changes

JimClarke5 added 6 commits March 4, 2021 16:11

Add GeLU activation present in TF 2.4

c5d37bf

Fix @param<T> and reformat

11f8ac9

Fix JavaDoc to add @param <T>

40a95af

Refactor to add generic to base class and change signature of call me…

d0e8de9

…thod to <U extends T>. Changed all subclasses to match these signatures.

Add check for scalar.

478b78a

Change to accept TString value.

f53fa08

Craigacp requested changes Mar 9, 2021

View reviewed changes

JimClarke5 added 5 commits March 9, 2021 15:29

Fix GeLU equations with separate Operands

79594da

Fix Constant to handle TString properly

112c740

added additional test case for invalid conversion of TString to TInt32

Added Stddev check for not less than 0.

61e6206

Fix fix fill to cast the 1 to the approriate type before the fill

3b4b607

Code reformat

98df654

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic cleanup rest of framework, activations and initializers #231

Generic cleanup rest of framework, activations and initializers #231

JimClarke5 commented Mar 3, 2021

karllessard left a comment

karllessard Mar 4, 2021

JimClarke5 Mar 4, 2021

Craigacp Mar 4, 2021

karllessard Mar 4, 2021

JimClarke5 Mar 4, 2021

JimClarke5 Mar 7, 2021

deansher Mar 8, 2021

Craigacp Mar 4, 2021

JimClarke5 Mar 4, 2021

Craigacp Mar 4, 2021

JimClarke5 Mar 6, 2021

JimClarke5 Mar 7, 2021 •

edited

Loading

Craigacp Mar 9, 2021

Craigacp Mar 4, 2021

JimClarke5 Mar 4, 2021

Craigacp Mar 4, 2021

deansher Mar 5, 2021

JimClarke5 Mar 5, 2021

Craigacp Mar 5, 2021

deansher Mar 8, 2021

Craigacp Mar 9, 2021

Craigacp left a comment

Craigacp Mar 9, 2021

JimClarke5 Mar 9, 2021

Craigacp Mar 9, 2021

Craigacp Mar 9, 2021

JimClarke5 Mar 9, 2021

Craigacp Mar 9, 2021

JimClarke5 Mar 9, 2021

Craigacp Mar 9, 2021

Craigacp Mar 9, 2021

Craigacp Mar 9, 2021

Craigacp Mar 9, 2021

Generic cleanup rest of framework, activations and initializers #231

Are you sure you want to change the base?

Generic cleanup rest of framework, activations and initializers #231

Conversation

JimClarke5 commented Mar 3, 2021

karllessard 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

Choose a reason for hiding this comment

JimClarke5 Mar 7, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Craigacp 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

Choose a reason for hiding this comment

JimClarke5 Mar 7, 2021 •

edited

Loading