Learning rate by JimClarke5 · Pull Request #106 · tensorflow/java

JimClarke5 · 2020-09-01T17:31:17Z

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.

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

deansher

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

deansher · 2020-09-17T13:15:10Z

tensorflow-core/tensorflow-core-api/src/main/java/org/tensorflow/DataType.java

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


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?

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.

deansher · 2020-09-17T13:37:13Z

tensorflow-framework/src/main/java/org/tensorflow/framework/optimizers/AdaDelta.java

+   * @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);


-> RHO_DEFAULT, EPSILON_DEFAULT

Never mind, in this context.

Sync

Craigacp · 2020-09-19T20:03:37Z

tensorflow-framework/src/main/java/org/tensorflow/framework/optimizers/Optimizer.java

+   * @param learningRate the learning rate.
   */
-  protected Optimizer(Graph graph, String name) {
+  protected Optimizer(Graph graph, float learningRate) {


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

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

CTORS have been changed

Craigacp · 2020-09-19T20:04:04Z

tensorflow-framework/src/main/java/org/tensorflow/framework/optimizers/Optimizer.java

    return variable.op().name() + "-" + slotName;
  }

-  /**


Why'd the Javadoc go away?

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

Update pushed

Craigacp · 2020-09-19T20:07:15Z

tensorflow-framework/src/main/java/org/tensorflow/framework/optimizers/Optimizer.java

    }
  }

-  /**


Where'd the javadoc go?

I have added it back in. Update pushed

Craigacp · 2020-09-19T20:13:49Z

tensorflow-framework/src/main/java/org/tensorflow/framework/optimizers/Optimizer.java

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


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.

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

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

Update pushed

…ewLearningRate), eliminated spurious "this."

Craigacp · 2020-09-21T18:51:01Z

tensorflow-framework/src/main/java/org/tensorflow/framework/optimizers/Optimizer.java

   * Sets the learning rate
   *
-   * @param learningRate the learning rate
+   * @param newLearningRate the new earning rate


typo - "earning"

tensorflow-framework/src/main/java/org/tensorflow/framework/optimizers/Optimizer.java

…raph graph, String name, float learningRate)"", change all the subclass ctors to use this one.

…feedMap to null.

Add Operand<TFloat32> learningRateOperand as an option for learning rate.

Craigacp

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

Craigacp · 2020-09-30T13:36:30Z

tensorflow-framework/src/test/java/org/tensorflow/framework/optimizers/RMSPropTest.java

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


Please switch the python style variable names to camelCase.

Craigacp · 2020-09-30T13:36:47Z

tensorflow-framework/src/test/java/org/tensorflow/framework/optimizers/AdamTest.java

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


Switch python style name to camelCase.

JimClarke5 · 2021-01-03T16:22:44Z

This PR requires some rework due to #174, Type Refactor, Should we close this PR and open a new one later? Also, this PR is closely related to callbacks, so I suggest we revisit this one after callbacks is done. My current plan is #180, Metrics Phase1, followed by Metrics Phase2, then Model/Layers Phase 1, then callbacks, then this PR can be revisited.

Craigacp · 2021-01-03T17:14:43Z

This PR requires some rework due to #174, Type Refactor, Should we close this PR and open a new one later? Also, this PR is closely related to callbacks, so I suggest we revisit this one after callbacks is done. My current plan is #180, Metrics Phase1, followed by Metrics Phase2, then Model/Layers Phase 1, then callbacks, then this PR can be revisited.

Whatever you think is easiest is fine by me.

JimClarke5 · 2021-05-28T16:28:51Z

I am closing for now, and will reopen after we get further along on Model.

JimClarke5 added 10 commits August 30, 2020 18:42

Add support for hanling feed dicts when evaluating or printing Operands.

0afdb9c

Add tests for changing learning rates

9f10da9

Moved Optimizers to Keras. Added support for chanign learning rate.

0cc4012

Moved Optimizers to Keras. Added support for chanign learning rate.

d8fab04

Reformatted code

dddc297

Reformatted code

cb8104c

Reformatted code

15189b4

Remove premature commit

eb2c48e

deansher suggested changes Sep 17, 2020

View reviewed changes

Merge pull request #2 from JimClarke5/master

a2b5d7b

Sync

Craigacp reviewed Sep 19, 2020

View reviewed changes

Added JavaDoc back in, changed setLearningRate() to setLearningRate(n…

d5edd35

…ewLearningRate), eliminated spurious "this."

Craigacp reviewed Sep 21, 2020

View reviewed changes

JimClarke5 added 4 commits September 21, 2020 15:00

Change Optimizer to only have one constructor, "protected Optimizer(G…

2f57c1d

…raph graph, String name, float learningRate)"", change all the subclass ctors to use this one.

Fixed close() routine to free up closed tensor in feedMap by setting …

e9e2b24

…feedMap to null.

Fix javadoc for references to Default values,

62ff85c

Add Operand<TFloat32> learningRateOperand as an option for learning rate.

Added Operand<TFloat32> learningRateOperand test case for learning rate.

ca1395e

Craigacp requested changes Sep 30, 2020

View reviewed changes

karllessard mentioned this pull request Oct 14, 2020

Add Losses #129

Merged

karllessard mentioned this pull request Feb 11, 2021

Add Constraints #197

Closed

JimClarke5 closed this May 28, 2021

                   }
                 }
-                /**

Conversation

JimClarke5 commented Sep 1, 2020

Uh oh!

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

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!

JimClarke5 commented Jan 3, 2021

Uh oh!

Craigacp commented Jan 3, 2021

Uh oh!

JimClarke5 commented May 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants