Skip to content

Add Activations #123

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 54 commits into from
Oct 24, 2020
Merged

Add Activations #123

merged 54 commits into from
Oct 24, 2020

Conversation

JimClarke5
Copy link
Contributor

This is a redone PR to make sure the Activations branch did not rely on the Initializers branch.

I have removed all the no-arg CTORs. The original motivation was that I found previously in looking at Keras layers is that objects like “activations”, “loss” and “metric" most likely would need to be constructed
before the TensorFlow Ops is available within the model, hence I make constructors without Ops and used setTF() so the Model could set it later. The model itself hides Ops tf from the user. We have decided to address this requirement later when we start looking at Model.

I fixed HardSigmoid, as I forgot to apply a clipByValue operation on the final result. Also fixed the Unit test.

I have verified all the Unit tests against their TF Python corresponding methods.

For Softmax, we eliminated the IllegalArgmentException on 1D input that was in the Keras version.

In Swish, there is a comment on not handling the grad argument from the corresponding Python class. This is a Python optimization and will not be implemented in Java, at least at this time.

…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()
…EntropyWitLogits()

Added tf.nn.sparesSoftmaxCrossEntropyWithLogits() and
tf.nn.raw.sparesSoftmaxCrossEntropyWithLogits()

Added tf.nn.sigmoidCrossEntropyWithLogits()
…yWithLogits.java to nn.raw,

added new versions of these to NnOps
…x JavaDoc. Change from snake case to camel case.
…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.
…gmoidCrossEntropyWithLogits.java, and SparseSoftmaxCrossEntropyWithLogits.java under package org.tensorflow.op.nn in
…asy inclusion of a default optimizer. Cleaned up JavaDoc
Copy link
Collaborator

@Craigacp Craigacp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny things to do with casing.

return LeakyRelu.create(tf.scope(), input, LeakyRelu.alpha(alpha));
}
if (threshold != 0) {
negativePart =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would you think about moving both the declaration and the initialization of negativePart down to immediately before it is used? I realize that would require a change to keep input as the original input instead of reusing it for intermediate calculations, but personally I'd see that as a feature not a bug.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps rename negativePart -> amountBelowThreshold? For one thing, negativePart is vague to me. For another, it's always positive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this is used here as a math term, but I found Wolfram MathWorld - Negative Part. Perhaps @Craigacp and @karllessard might want to comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, that Wolfram definition does show why negativePart was used in the first place. That said, I'd still advocate that in the presence of threshold, it's a confusing name.

if (clipMax) {
Operand<T> lmaxValue = tf.dtypes.cast(tf.constant(maxValue), dataType);
Operand<T> zero = tf.dtypes.cast(tf.constant(0), dataType);
input = tf.clipByValue(input, zero, lmaxValue);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is zero still the right lower clip if there's a threshold?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The numbers less than the threshold get converted to zero before the clipMax is applied.
So, there is no need to move the lower bound on the clipByValue operation.

if (threshold != 0) {
      // computes input for input > threshold else 0
      Greater greater = tf.math.greater(input, tf.dtypes.cast(tf.constant(threshold), dataType));
      input = tf.math.mul(input, tf.dtypes.cast(greater, dataType));


/** Test of ELU call method */
@Test
public void testCallFloat() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps also consolidate this sort of test logic into a helper class? E.g.

assertFloat32Output(tf -> new ELU<Float32>(tf),
            {1f, -0.86466473f, 3f, -0.9816844f, -0.63212055f, 2f, -0.95021296f, 4f},
            {1, -2, 3, -4, -1, 2, -3, 4})
            

@karllessard karllessard mentioned this pull request Oct 14, 2020
if (threshold != 0) {
// computes input for input > threshold else 0
Greater greater = tf.math.greater(input, tf.dtypes.cast(tf.constant(threshold), dataType));
input = tf.math.mul(input, tf.dtypes.cast(greater, dataType));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would try to avoid changing the value of a method parameter, better use a new variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

Copy link
Collaborator

@karllessard karllessard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JimClarke5 , it looks like this PR can almost be merged, can you please go through all remaining comments and resolve them, either by a fix or by an answer? Thanks

Misc fixes to JavaDoc.
In ReLU, change to assign to new variable 'lInput' rather than change the 'input' parameter.
@JimClarke5
Copy link
Contributor Author

I have just pushed the changes requested, the biggest was adding TFloating in the core-api.

Copy link
Collaborator

@karllessard karllessard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Craigacp @deansher , there are a few discussions you've started in this PR that is still open, for me it looks like minor changes but can you can please have a quick check and give us your blessing to merge it in its current state or not?

Copy link
Contributor

@deansher deansher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ship it! This is a huge step forward.

@karllessard
Copy link
Collaborator

Thanks @JimClarke5 !

@karllessard karllessard merged commit 6f3ec0f into tensorflow:master Oct 24, 2020
@JimClarke5 JimClarke5 deleted the Activation branch October 27, 2020 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants