Generic cleanup rest of framework, activations and initializers by JimClarke5 · Pull Request #231 · tensorflow/java · GitHub

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

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

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.

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.)

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.

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.

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

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.

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.

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

OK

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.

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

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

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

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

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

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

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

-                      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));

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

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

+                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());
+                  }

                   }
-                  return tf.fill(dims, tf.dtypes.cast(tf.constant(1.0), type));
+                  return cast(tf, tf.fill(dims, tf.constant(1)), type);

-                      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())),

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

Generic cleanup rest of framework, activations and initializers by JimClarke5 · Pull Request #231 · tensorflow/java · GitHub

Uh oh!

Generic cleanup rest of framework, activations and initializers - #231

Generic cleanup rest of framework, activations and initializers#231JimClarke5 wants to merge 24 commits intotensorflow:mastertensorflow/java:masterfrom JimClarke5:Generic_cleanup_rest_of_fmworkJimClarke5/java:Generic_cleanup_rest_of_fmworkCopy head branch name to clipboard

Conversation

JimClarke5 commented Mar 3, 2021

Uh oh!

karllessard 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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

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!

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!

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!

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!

Generic cleanup rest of framework, activations and initializers#231
JimClarke5 wants to merge 24 commits into
tensorflow:masterfrom
JimClarke5:Generic_cleanup_rest_of_fmwork

JimClarke5 Mar 7, 2021 •
edited
Loading