Add Regularizers 1 #216

JimClarke5 · 2021-02-11T23:45:38Z

Add Regularizers.

Regularizers are classes that allow you to apply penalties on layer parameters or layer activity during
optimization. These penalties are summed into the loss function that the network optimizes.

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

Make sure Tests test TFloat32 1nd TFloat64

deansher · 2021-02-13T12:57:19Z

tensorflow-framework/src/main/java/org/tensorflow/framework/regularizers/L1L2.java

+ */
+public class L1L2<R extends TNumber> extends Regularizer<R> {
+
+  private final Float l1;


What's the motivation for storing these as Float instead of float? So far, we carefully set them to 0.0 instead of null, and then also carefully treat null as though it were 0.0.

Basically, L1L2, besides standing on its own, is the super class for L1_L2, L1, and L2. We still need the Float for the constructor because null is a valid value for either l1, or l2, depending in the subclass.
However, I will change the internal representation to float.

On second thought, maybe we don't need the Float. Just change the ctors for L1 and L2 to pass 0 for the unused calculation.

deansher · 2021-02-13T13:01:53Z

tensorflow-framework/src/main/java/org/tensorflow/framework/regularizers/L1L2.java

+
+    if (this.getL2() != null && this.getL2() != 0.f) {
+      Operand<R> l2Op = tf.dtypes.cast(tf.constant(this.getL2()), input.type());
+      Operand<R> sqr = tf.math.abs(input);


abs -> square

deansher · 2021-02-13T13:08:29Z

tensorflow-framework/src/main/java/org/tensorflow/framework/regularizers/L1L2.java

+ *
+ * <pre>loss = l2 * reduceSum(square(x))</pre>
+ *
+ * <p>The difference between this class and the {@link L1_L2} is use of the default regularization


The default of no penalty doesn't seem useful. I wonder if L1_L2 was created later in Python to provide useful defaults? Is it possible we should omit L1L2 on the Java side?

Actually it looks like l1_l2 was the after thought in Keras, as it is defined as a method. I am ok with adding the l1_l2 defaults to L1L2 and perhaps add a static method to L1L2 to create an L1L2 class with the current l1_l2 defaults.

Perhaps something like:

public static L1L2 l1_l2() { return new L1L2(DEFAULT_REGULARIZATION_PENALTY, DEFAULT_REGULARIZATION_PENALTY); }

That general approach seems fine to me. I do like the L1L2 class name better. The method name l1_l2 is off-Java-naming-standard and is descriptive only by reference to Python Keras. Perhaps instead overload the name create and use default values for the variant with no loss factors provided?

create or createDefault? Alternatively, I could change the constructors that do not specify the l1, l2 values,

public L1L2(Ops tf) { this(tf, DEFAULT_REGULARIZATION_PENALTY, DEFAULT_REGULARIZATION_PENALTY); }

I would lean toward changing the no-specified-penalty constructors to use the defaults instead of 0.f. But if factory methods felt more comfortable to you, and if you felt that fit some overall direction of the code base, I could be comfortable with that.

Yeah, I started down the createDefault path, but realized the idiomatic pattern is just overloaded create methods.

I am changing L1L2(Ops tf) to use t DEFAULT_REGULARIZATION_PENALTY and eliminating class L1_L2.

deansher · 2021-02-13T13:22:09Z

tensorflow-framework/src/test/java/org/tensorflow/framework/regularizers/L1L2Test.java

+        Operand<TFloat32> weights = tf.constant(w);
+        Operand<TFloat32> result = instance.call(weights);
+        float expected = regularizeL1L2(w, 0.01f, 0.02f);
+        session.setEpsilon(.09f);


This tolerance seems unreasonably high to me. I imagine it is why this test passes in spite of the use of abs instead of square to compute the L2 loss.

In python, if I've followed the code path correctly, they use:

def assertAllClose(self, a, b, rtol=1e-6, atol=1e-6, msg=None):

where

The relative difference (rtol * abs(b)) and the absolute difference atol are added together to compare against the absolute difference between a and b.

There seems to be a wide tolerance needed when calculating in Java vs calculation in TF. Not sure why, but seems to happen a lot.

That seems concerning to me! For most (reasonably stable) computations, for the first N significant digits, there's only one right answer. Or am I missing something?

I am not sure, but I have seen a wide difference in the number of significant digits in other test cases when I use Java to determine the expected result.

👍 Ok, seems this is a broader issue, not part of this PR. I'll raise an issue.

I do notice that if I use NumPy to do the test calculation beforehand, rather than use Java to do the calculation at runtime, things are closer to TF. I am not sure if my calculations don't exactly match NumPy behavior or not. I have found subtle differences between Java and Python behavior in other areas when it comes to Math which may be part of the problem. Negative 0 being one of them.

deansher · 2021-02-13T13:24:49Z

tensorflow-framework/src/main/java/org/tensorflow/framework/regularizers/Regularizer.java

+ *
+ * @param <R> the data type of the operands and result
+ */
+public abstract class Regularizer<R extends TNumber> {


Is there a clear motivation for giving Regularizer a type parameter? For today's existing regularizers, L1 and L2, it would be sufficient to put a type parameter on call. Is there reason to think the Regularizer instance itself needs to be specialized to a certain tensor type?

I will fix.

deansher · 2021-02-13T14:28:01Z

tensorflow-framework/src/test/java/org/tensorflow/framework/utils/ND.java

+    if (keepDims) {
+      long[] newDims = shape.asArray();
+      newDims[axis] = 1;
+      final AtomicInteger counter = new AtomicInteger();


Why the AtomicInteger here? It is local.

It is referenced in the lambda on lines 784-789, you can't use int i because it is not final, and if it were final you couldn't increment it.

👍 Oh, right!

deansher · 2021-02-13T14:44:06Z

tensorflow-framework/src/test/java/org/tensorflow/framework/utils/ND.java

+    int xis = nDims - 1 - axis;
+    long totalSize = shape.size();
+    long axisSize = shape.size(xis);
+    final double[] sums = new double[(int) axisSize];


Given that sums is one-dimensional, maybe this method only works for 2d arrays?

ND is a quick and dirty replacement for NumPy just used in testing. If we ever do a cheap version of NumPy, then this class can be replaced.

Should we move ND into src/test to avoid setting expectations we don't intend to meet?

On my fork, it is in the test directory ??? test/java/org/tensorflow/framework/utils

Oh, that's where it is!

Perhaps rename this method sum2D and validate a accordingly?

I think this goes to the broader issue, are we going to fully implement a NumPy Replacement or not.

👍 Fine -- it's a test class

deansher · 2021-02-13T14:47:19Z

tensorflow-framework/src/test/java/org/tensorflow/framework/utils/ND.java

+              .elements(newDims.length - 1)
+              .forEachIndexed(
+                      (idx, v) -> {
+                        v.setDouble(sums[counter.getAndAdd(1)]);


forEachIndexed does not guarantee an iteration order, so I don't think keeping a parallel counter works.

Seems to work, I suppose you could take the axis index out of idx, and use that. Again this class is just used for testing.

👍 Fine -- it's a test class

deansher · 2021-02-13T14:48:03Z

tensorflow-framework/src/test/java/org/tensorflow/framework/utils/ND.java

+  }
+
+  /**
+   * Sum all elements of an array based on the specified axis


Perhaps be more specific by saying "along" or "over" instead of "based on"?

👍 Fine -- it's a test class

deansher · 2021-02-13T14:49:47Z

tensorflow-framework/src/test/java/org/tensorflow/framework/utils/ND.java

+      DoubleNdArray result = sum(a);
+      if (keepDims) {
+        double scalar = result.getDouble(0);
+        long[] dims = {1, 1};


Only supports 2d arrays.

By design right now, I just did enough for testing.

A NumPy replacement based on ndarray, would be a nice feature to have if you want to explore more.
This was brought up at yesterday's meeting. TensorFlow SIG JVM Notes

Perhaps rename this method sum2D and validate a accordingly?

👍 Fine -- it's a test class

Removed generic from Regularizer class and changed the call method to define the generic return based on the weights parameter. Added static method l1_l2() to L1L2 class. Fixed JavaDoc comments.

modified Float to float for l1 and l2 parameters Change ctor L1L2(Ops tf) to use DEFAULT_REGULARIZATION_PENALTY for l1/l2 parameters Fix JavaDoc

deansher

I have a few open questions, but I don't see any of those as essential for this PR.

Merge with latest

Resync with origin/master

Sync with tensorflow/java master

Sync

Sync with Metrics Phase 2

karllessard · 2021-04-26T13:42:08Z

@JimClarke5 , can you please rebase that PR so we can validate that it now passes the quick build successfully?

karllessard · 2021-04-28T02:50:12Z

So I reran the same job and it is still failing. So after your rebase, we'll see if the compilation passes and merge it right away if that's the case, thanks

karllessard · 2021-05-01T20:20:15Z

@JimClarke5 , just a small reminder, if you can find a few minutes to rebase/resubmit that PR so I can merge it, thanks!

Sync with master

JimClarke5 · 2021-05-01T22:17:55Z

@karllessard I just pushed the rebased versions based on the latest Master. It seemed to push a lot of core files, so if you don't want that, I will create a clean branch of Regularizers and create a new PR.

karllessard · 2021-05-02T02:05:53Z

@JimClarke5 , I think the rebase went fine, only files of the framework are part of the PR. On the other hand, compilation is failing with the following errors, can you please check?

Error:  COMPILATION ERROR : 
3468
[INFO] -------------------------------------------------------------
3469
Error:  /__w/java/java/tensorflow-framework/src/test/java/org/tensorflow/framework/utils/ND.java:[819,31] method sum(org.tensorflow.ndarray.DoubleNdArray) is already defined in class org.tensorflow.framework.utils.ND
3470
Error:  /__w/java/java/tensorflow-framework/src/test/java/org/tensorflow/framework/utils/ND.java:[832,31] method sum(org.tensorflow.ndarray.DoubleNdArray,int) is already defined in class org.tensorflow.framework.utils.ND
3471
Error:  /__w/java/java/tensorflow-framework/src/test/java/org/tensorflow/framework/utils/ND.java:[844,31] method sum(org.tensorflow.ndarray.DoubleNdArray,int,boolean) is already defined in class org.tensorflow.framework.utils.ND

JimClarke5 · 2021-05-02T11:15:02Z

@karllessard weird because I had fixed that error and compiled on my side. I will re look at it.

Make sure Tests test TFloat32 1nd TFloat64

Removed generic from Regularizer class and changed the call method to define the generic return based on the weights parameter. Added static method l1_l2() to L1L2 class. Fixed JavaDoc comments.

modified Float to float for l1 and l2 parameters Change ctor L1L2(Ops tf) to use DEFAULT_REGULARIZATION_PENALTY for l1/l2 parameters Fix JavaDoc

# Conflicts: # tensorflow-framework/src/main/java/org/tensorflow/framework/regularizers/RegularizerLoss.java

JimClarke5 · 2021-05-02T12:43:56Z

@karllessard totally weird because my local copy was fixed but the upstream copy was not. IntelliJ did not indicate that it needed to be committed. I fooled around with it by cleaning up some code and repushed and now the upstream copy is the same as the local copy. Try it now.

karllessard · 2021-05-02T14:03:04Z

Hey @JimClarke5 , don’t know what happened neither but rerunning the build make it pass this time. I’m merging, thanks again for your contribution!

karllessard · 2021-05-02T14:05:17Z

@karllessard totally weird because my local copy was fixed but the upstream copy was not. IntelliJ did not indicate that it needed to be committed. I fooled around with it by cleaning up some code and repushed and now the upstream copy is the same as the local copy. Try it now.

Oh ok, just noticed that reply of yours, that explains it ;)

JimClarke5 added 11 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

Initial Checkin

ccc7820

Clean up JavaDoc

05ec6e8

Make sure Tests test TFloat32 1nd TFloat64

Fix to match the lates version of losses.Loss

b446618

deansher suggested changes Feb 13, 2021

View reviewed changes

JimClarke5 added 4 commits February 13, 2021 13:45

Updates based on comments from PR.

b5c7c78

Removed generic from Regularizer class and changed the call method to define the generic return based on the weights parameter. Added static method l1_l2() to L1L2 class. Fixed JavaDoc comments.

Add JavDoc to new method l1_l2

a3ccf61

change l1_l2 to create.

8c79214

delete class L1_L2

1af4552

modified Float to float for l1 and l2 parameters Change ctor L1L2(Ops tf) to use DEFAULT_REGULARIZATION_PENALTY for l1/l2 parameters Fix JavaDoc

deansher previously approved these changes Feb 17, 2021

View reviewed changes

JimClarke5 added 5 commits February 23, 2021 18:17

Merge pull request #11 from tensorflow/master

e038bbd

Merge with latest

Merge pull request #13 from tensorflow/master

def3051

Resync with origin/master

Merge pull request #15 from tensorflow/master

11748ae

Sync with tensorflow/java master

Merge pull request #16 from tensorflow/master

a9412ea

Sync

Merge pull request #17 from tensorflow/master

2ff8dfe

Sync with Metrics Phase 2

karllessard added CI build and removed CI build labels Apr 28, 2021

JimClarke5 added 2 commits May 1, 2021 16:40

Merge pull request #18 from tensorflow/master

ee5e38a

Sync with master

Rebase with tensorflow Master

54f1802

karllessard added CI build and removed CI build labels May 2, 2021

JimClarke5 added 7 commits May 2, 2021 08:33

Updating fixed local copy to repair broken remote copy

bbd3bc3

Clean up JavaDoc

6c48131

Make sure Tests test TFloat32 1nd TFloat64

Updates based on comments from PR.

3c45a87

Removed generic from Regularizer class and changed the call method to define the generic return based on the weights parameter. Added static method l1_l2() to L1L2 class. Fixed JavaDoc comments.

Add JavDoc to new method l1_l2

2bd80b3

change l1_l2 to create.

da7a10b

delete class L1_L2

9ea1d9a

modified Float to float for l1 and l2 parameters Change ctor L1L2(Ops tf) to use DEFAULT_REGULARIZATION_PENALTY for l1/l2 parameters Fix JavaDoc

Merge remote-tracking branch 'origin/Regularizers_1' into Regularizers_1

1a93bdc

# Conflicts: # tensorflow-framework/src/main/java/org/tensorflow/framework/regularizers/RegularizerLoss.java

JimClarke5 dismissed deansher’s stale review via 1a93bdc May 2, 2021 12:40

karllessard approved these changes May 2, 2021

View reviewed changes

karllessard merged commit e013353 into tensorflow:master May 2, 2021

JimClarke5 deleted the Regularizers_1 branch May 2, 2021 15:00

Add Regularizers 1 #216

Add Regularizers 1 #216

Uh oh!

Conversation

JimClarke5 commented Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JimClarke5 Feb 14, 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!

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!

deansher Feb 14, 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!

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!

Choose a reason for hiding this comment

Uh oh!

JimClarke5 Feb 14, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

JimClarke5 commented Feb 11, 2021 •

edited

Loading

JimClarke5 Feb 14, 2021 •

edited

Loading

deansher Feb 14, 2021 •

edited

Loading

JimClarke5 Feb 14, 2021 •

edited

Loading