close
The Wayback Machine - https://web.archive.org/web/20201209105706/https://github.com/tensorflow/java/pull/106
Skip to content
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

Learning rate #106

Open
wants to merge 51 commits into
base: master
from
Open

Learning rate #106

wants to merge 51 commits into from

Conversation

@JimClarke5
Copy link
Contributor

@JimClarke5 JimClarke5 commented Sep 1, 2020

This PR requires PR "Initial checkin of Keras Optimzers and helper classes" to be merged first.

Added changeable learning rate to Optimizers. This was done by adding a Placeholder for the learning rate, a Tensor to track the actual learning rate, and adding a Map to map the Placeholder to the Tensor that can be used to "feed" the runner.
Test Sessions were modified to accept a "FeedDict" Map to popullate the feed() of the runner.

JimClarke5 added 30 commits Jul 28, 2020
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
fix javadoc, fix casts
…yWithLogits.java to nn.raw,

added new versions of these to NnOps
…maxCrossEntropyWithLogits()
…maxCrossEntropyWithLogits()
…lder into each Optimizer.

Also, added to each Optimizer a corresponding Tensor that holds the value of the learning rate, and added a feed dictionary that maps the placeholder to the Tensor, so that it can be fed into the runner when running or evaluating. When setLearning rate is called the learning rate tensor and the feed dictionary are updated.
…lder into each Optimizer.

Also, added to each Optimizer a corresponding Tensor that holds the value of the learning rate, and added a feed dictionary that maps the placeholder to the Tensor, so that it can be fed into the runner when running or evaluating. When setLearning rate is called the learning rate tensor and the feed dictionary are updated.
…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.
JimClarke5 added 14 commits Sep 3, 2020
…gmoidCrossEntropyWithLogits.java, and SparseSoftmaxCrossEntropyWithLogits.java under package org.tensorflow.op.nn in
…asy inclusion of a default optimizer. Cleaned up JavaDoc
…cases to framework. Created Tests for GradientDescent and Momentum
Copy link
Contributor

@deansher deansher left a comment

A couple of quick observations as I start reading this code.

}

/** Returns true if this data type represents a floating point type */
public boolean isFloating() {

This comment has been minimized.

@deansher

deansher Sep 17, 2020
Contributor

This pattern is very uncomfortable to me: DataType being omniscient about TType and dispatching on a string NAME. What's our motivation? If we think it's the best pattern for this situation, perhaps we could document why?

This comment has been minimized.

@deansher

deansher Sep 19, 2020
Contributor

Never mind, in this context -- I see in my local diff that this delta is unrelated to this PR. I'll raise this as an issue.

* @param graph the TensorFlow Graph
* @param name the name for this Optimizer (defaults to 'Adadelta')
* @param learningRate the learning rate
*/
public AdaDelta(Graph graph, String name, float learningRate) {
this(graph, name, learningRate, 0.95f, 1e-8f);

This comment has been minimized.

@deansher

deansher Sep 17, 2020
Contributor

-> RHO_DEFAULT, EPSILON_DEFAULT

This comment has been minimized.

@deansher

deansher Sep 19, 2020
Contributor

Never mind, in this context.

JimClarke5 added 2 commits Sep 18, 2020
Sync Clarke fork
Sync
*/
protected Optimizer(Graph graph, String name) {
protected Optimizer(Graph graph, float learningRate) {

This comment has been minimized.

@Craigacp

Craigacp Sep 19, 2020
Collaborator

Can we have both of these constructors call into the Optimizer(Graph,float,String) one?

This comment has been minimized.

@JimClarke5

JimClarke5 Sep 19, 2020
Author Contributor

Being that Optimizer is abstract, we really only need one constructor, protected Optimizer(Graph graph, String name, float learningRate) . Of course, we would have to handle a null name, with something like:
this.tf = Ops.create(graph).withName(name == null? getOptimizerName() : name);

This comment has been minimized.

@JimClarke5

JimClarke5 Sep 23, 2020
Author Contributor

CTORS have been changed

public static String createName(Output<? extends TType> variable, String slotName) {
return variable.op().name() + "-" + slotName;
}

/**

This comment has been minimized.

@Craigacp

Craigacp Sep 19, 2020
Collaborator

Why'd the Javadoc go away?

This comment has been minimized.

@JimClarke5

JimClarke5 Sep 19, 2020
Author Contributor

I am not sure what happened. I had a local copy that I saved and it was there, so will add it back in.

This comment has been minimized.

@JimClarke5

JimClarke5 Sep 19, 2020
Author Contributor

Update pushed

@@ -305,41 +350,20 @@ private Options() {}
}
}

/**

This comment has been minimized.

@Craigacp

Craigacp Sep 19, 2020
Collaborator

Where'd the javadoc go?

This comment has been minimized.

@JimClarke5

JimClarke5 Sep 19, 2020
Author Contributor

I have added it back in. Update pushed

* @param learningRate the learning rate
*/
public final void setLearningRate(float learningRate) {
if (this.learningRatePlaceholder == null) {

This comment has been minimized.

@Craigacp

Craigacp Sep 19, 2020
Collaborator

Everything seems to have grown a this reference. I don't think that's particularly necessary in these methods, as the argument could be newLearningRate rather than learningRate and then there is no aliasing.

This comment has been minimized.

@JimClarke5

JimClarke5 Sep 19, 2020
Author Contributor

I do this out of habit. I can easily change it as you suggest.

This comment has been minimized.

@JimClarke5

JimClarke5 Sep 19, 2020
Author Contributor

changed setLearningRate to setLearningRate(float newLearningRate), removed spurious this..

Update pushed

…ewLearningRate), eliminated spurious "this."
@@ -280,20 +321,20 @@ protected Op finish(List<Op> updateOperations, String name) {
/**
* Sets the learning rate
*
* @param learningRate the learning rate
* @param newLearningRate the new earning rate

This comment has been minimized.

@Craigacp

Craigacp Sep 21, 2020
Collaborator

typo - "earning"

This comment has been minimized.

@JimClarke5

JimClarke5 Sep 23, 2020
Author Contributor

OK

JimClarke5 added 4 commits Sep 21, 2020
…raph graph, String name, float learningRate)"", change all the subclass ctors to use this one.
Add Operand<TFloat32> learningRateOperand as an option for learning rate.
Copy link
Collaborator

@Craigacp Craigacp left a comment

Just a few snake_case variable names in the tests that need converting to camelCase, and then I'll merge this in.

new RMSProp(session.getGraph(), learningRate, decay, momentum, epsilon, centered)) {
Ops tf = session.getTF();
session.setEpsilon(1e-2f);
float[] var0_init = {1.0F, 2.0F};

This comment has been minimized.

@Craigacp

Craigacp Sep 30, 2020
Collaborator

Please switch the python style variable names to camelCase.

FloatNdArray mul1 = ND.mul(v, beta);
FloatNdArray squareG = ND.square(gT);
FloatNdArray mul2 = ND.mul((1 - beta), squareG);
return ND.add(mul1, mul2);
}

private FloatNdArray calculateParam(
FloatNdArray param, float lrT, FloatNdArray m, FloatNdArray v, float epsilon) {
// param - lrT * mT / (np.sqrt(vT) + epsilon)
FloatNdArray param, float lr_t, FloatNdArray m, FloatNdArray v, float epsilon) {

This comment has been minimized.

@Craigacp

Craigacp Sep 30, 2020
Collaborator

Switch python style name to camelCase.

@karllessard karllessard mentioned this pull request Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.