perhaps "of the predictions and result"?

OK

The Tuple class name is uncomfortably vanilla for me. Perhaps LossTuple?

This object will also be used in Metrics as many metrics are built using loss classes or Losses methods. I have changed it to LossTuple.

The Javadocs in this file are still partly in markdown.

OK, I thought I caught them all, I will fix.

For this method, the returned sampleWeight is always null.

That is not always the case when we do Metrics.

I'm just thinking our documentation for this method might take into account that the returned sampleWeight is always null.

Now I see what you are talking about. I added a comment in the @return that sampleWeight will be null for this particular method signature.

Is "match" the right way to describe the precondition relationship between predictions and labels?

It is definitely not the same Shape. I was thinking of compatible, but that has specific meaning in Shape.isCompatibleWIth. The description is saying the ranks must be equal or differ by one. I am not sure of one word that describes that. match was the word used in the Python version of this method.

Hmm, here's a suggestion:

• We could decide what we want the convention to be, in terms of squeeze-or-expand plus maybe broadcasting.
• Write this up carefully in the class javadoc for either Loss or Losses.
• Mention that documentation in the class javadoc for every other loss class.
• Also mention it in Loss#call.
• And be silent about it in the individual methods of Losses and LossesImpl.
  Perhaps?

That said, it just occurred to me that we have another gap, and that filling that gap might help this issue.

We don't specify the behavior of these methods when labels and predictions don't have a permitted shape relationship. Nor do we make sure our behavior is consistent in that case.

Perhaps we should

• spell out that there's an IllegalArgumentException for that in the statically-known-dimensions case,
• rename squeezeOrExpandDimensions into something like validateAndAdjustLossDimensions,
• have that method throw IllegalArgumentException when appropriate,
• and then link to a fuller explanation in the documentation of the IllegalArgumentException?

Although I have never been in the habit of subclassing IllegalArgumentException, I see Oracle does that sometimes. That could be an alternative way of pointing people to the fuller explanation.

match must mean that the shapes of the input operands are capable of being molded into the relationships defined for the result of this method. Again LossesImpl is intended to be marked as module private (JDK 11) and only should be accessible from the losses or metrics package. It is not intended to be a general use API.

We should probably note in the javadoc for the class that this is an internal implementation class and subject to change (and being locked off under the module system).

Added this comment for the LossesImpl class

/**
 * These are helper methods for Losses and will be module private when
 * Java modularity is applied to TensorFlow Java.
 * These methods should not be used outside of the Loss package.
 */

For consistency, labelsRank.

OK

Extraneous *

Deleted

Extraneous *

Removed all Extraneous @link *

. . . , or null to use {@link Class#getSimpleName()}

Why would someone want to pass null, when there are other CTORs that handle that condition?

For APIs that will get enough use to be worth some polish, I tend toward carefully documenting edge cases. I don't know whether we want to invest in that now.

I think it's worth documenting it in case users build their own losses.

OK, added this to name param, if null the name will be {@link Class#getSimpleName()}.

. . . , or null to use {@link Class#getSimpleName()}

OK, added this to all name param, if null the name will be {@link Class#getSimpleName()}.

Actually, there's a separate <U> for the labels.

Fixed

Although in this internal case of this method, we do broadcast. I'll stop commenting on this issue.

We can return to the "squeezeOrExpandDimensions followed by broadcasting" topic when I work on #130 .

Resolved.

Do we want to avoid this cast in the case where labels already has the same data type?

I guess the question is what is the overhead of casting onto oneself vs the overhead of checking? I would hope that tf.dtypes.cast already handles this, but I could be mistaken.

The code for checking could be something like this:

@SuppressWarnings("unchecked")
private static <T extends TNumber, U extends TNumber> Operand<T> castIfNecessary(
    Operand<U> value, DataType<T> requiredType) {
  return (value.asOutput().dataType() == requiredType) 
      ? (Operand<T>) value
      : tf.dtypes.cast(value, requiredType);
}

So the overhead of checking would be the function call plus value.asOutput().dataType() == requiredType.

Looking at the code for tf.dtypes.cast, unless we think a cast is almost always needed, it would be cheaper to do the check to sometimes avoid it.

  public <U extends TType, T extends TType> Cast<U> cast(Operand<T> x, DataType<U> DstT,
      Cast.Options... options) {
    return Cast.create(scope, x, DstT, options);
  }

  @Endpoint(describeByClass = true)
  public static <U extends TType, T extends TType> Cast<U> create(Scope scope, Operand<T> x, DataType<U> DstT, Options... options) {
    OperationBuilder opBuilder = scope.env().opBuilder("Cast", scope.makeOpName("Cast"));
    opBuilder.addInput(x.asOutput());
    opBuilder = scope.applyControlDependencies(opBuilder);
    opBuilder.setAttr("DstT", DstT);
    if (options != null) {
      for (Options opts : options) {
        if (opts.Truncate != null) {
          opBuilder.setAttr("Truncate", opts.Truncate);
        }
      }
    }
    return new Cast<U>(opBuilder.build());
  }

In graph construction mode the overhead is probably irrelevant because it's only called once during construction. In eager mode it could be faster as it could sidestep a JNI call in each step, but I suspect we've got other issues to get speed in eager mode.

I like castIfNecessary as a general util method. It would be used almost everywhere, so it would be a huge change.
Perhaps create a new PR for castIfNecessary, then once that is merged we can start retrofitting all packages under framework.

In graph construction mode, an unnecessary call to cast creates an unnecessary graph operation.

shrug it'll be a no-op most of the time and compiled away if we get XLA working. Given the relative size of the computation around it I suspect it won't be an issue.

I also vote for a explicit check in the code to avoid adding an extra operation to the graph when it is not required

OK, I will add a helper class in org.tensorflow.framework.utils, then retrofit the Loss classes.

Just a comment on @deansher proposed method here, the datatypes for <U> and <T> should not be restricted to TNumber because it is valid to cast to/from TNumber and TBool .

Can just use dataType.

Fixed

I tripped over this private method having the usual naming of a loss method, since I didn't notice that it was private and so expected it to follow the conventions of public loss methods, such as invoking squeezeOrExpandDimensions. Also (if I'm navigating accurately through unfamiliar territory), this method doesn't compute a binaryCrossentropy since it depends on its caller to compute the mean at the end.

This method does the grunt work for the binaryCrossentropy after the operands have had their shapes and types manipulated and after smoothing the labels. Perhaps a new name would remove some of the confusion.

Yes, I wonder if we want to call it something like binaryCrossentropyHelper?

OK, Changed

Should we follow the Python in documenting that labels are expected to be 0 or 1?

Yes, The Python CategporicalHinge class does not mention that at all, but it is mentioned in the categorical_hinge method.

I have added an entry to the class JavaDoc and to the Losses.categoricalHinge method.

Actually the values can be [-1, 0, 1]. [0,1] is converted to [-1,1]. I have added a value check to make sure the values are wholly contained in the allowed values set [-1, 0, 1]. This will either throw TFInvalidArgumentException if run in Graph mode via a control dependency, and throw IllegalArgumentException if created in Eager mode with the call method.

Cool -- Resolved.

What does it do if there are all three of [-1 0 1] present? That's probably an invalid input, does it throw?

Could just use dataType. I'll stop mentioning this.

Fixed, hopefully I have fixed them all.

I think "percentage" is extraneous here.

Fixed

That said, it just occurred to me that we have another gap, and that filling that gap might help this issue.

We don't specify the behavior of these methods when labels and predictions don't have a permitted shape relationship. Nor do we make sure our behavior is consistent in that case.

Perhaps we should

• spell out that there's an IllegalArgumentException for that in the statically-known-dimensions case,
• rename squeezeOrExpandDimensions into something like validateAndAdjustLossDimensions,
• have that method throw IllegalArgumentException when appropriate,
• and then link to a fuller explanation in the documentation of the IllegalArgumentException?

Although I have never been in the habit of subclassing IllegalArgumentException, I see Oracle does that sometimes. That could be an alternative way of pointing people to the fuller explanation.

If the rank is unknown, then the size of the last dimension is guaranteed to be unknown, so isCompatible is guaranteed true. (But there may be some idiomatic reason for writing it this way, of which I am blissfully unaware.)

Correct, it should have been or not and.

I'm pretty sure this logic is wrong. Perhaps either

• document preconditions of removeSqueezableDimensions and check exactly those,
• or (my leaning) just invoke removeSqueezableDimensions and make it however smart it needs to be.

This logic is checking to see if both objects ranks are known (not Shape.unknown()). If both ranks are known, then it checks to see if the shapes are already in the right relationship or not. If not in the right relationship, then call removeSqueezableDimensions. It is basically an optimization to avoid doing the work in removeSqueezableDimensions if it does not need to be done.

rank could be -1 at this point.

Fixed

allAxes?

Changed name to allAxes

What should happen if weightsRank is UNKNOWN?

It falls through and executes the last part of the method after the // Use dynamic rank. comment.

:-) Oh yeah, that.

Here we are working with the original predictionsRank, when we wanted to be working with the new rank.

This matches the original Python code, but when you think about it, the predictions rank would never change from UNKNOWN to KNOWN and vice versa in a static context.

I was thinking perhaps predictions changed rank through our squeezing it to match labels earlier in the method. But I think there's a more pernicious problem. Here's an elided version of some of the method's code. Notice that we may squeeze predictions, but we only store the result in tuple. If we then also work with sampleWeight, we neither reference the squeezed version of predictions nor return it.

    if (labels != null) {
        . . . 
        if (predictionsRank - labelRank != 1 || predictionsShape.size(-1) == 1) {
          tuple = removeSqueezableDimensions(tf, labels, predictions);
        }
      } else { // use dynamic rank
        tuple = removeSqueezableDimensions(tf, labels, predictions);
      }
    }
    . . .
    if (predictionsRank != Shape.UNKNOWN_SIZE && weightsRank != Shape.UNKNOWN_SIZE) {

      if (weightsRank - predictionsRank == 1) {
        sampleWeight = tf.squeeze(sampleWeight);
        . . .
      }
      return new Tuple<>(labels, predictions, sampleWeight);
    }

Yes, we should probably fetch the labels and predictions from tuple first. I'll fix it.

This method has a myriad of complex cases, so I think it deserves its own direct unit test.

I could not find direct test cases for this method in Python. It's defined in tensorflow/tensorflow/python/ops/losses/utils.py. You want to take a stab at it?

:-) Totally. I want to do some work on #92 first, so I'll open an issue for myself.

Does labelSmoothing = 1.0 mean the true label distribution is set to 1/n? I'm not sure what "squeezing the values towards 0.5" means, because it would only be 0.5 in a binary problem.

Actually this is the comment for BinaryCrossentropy. It should be:

Float in <code>[0, 1]</code>. When <code>&gt; 0</code>, label values are smoothed, meaning the
confidence on label values are relaxed. e.g. <code>label_smoothing=0.2<code> means that we will use a
value of </code>0.1<code> for label </code>0<code> and </code>0.9<code> for label </code>1<code>

I'll fix it.

I think it's worth documenting it in case users build their own losses.

We should probably note in the javadoc for the class that this is an internal implementation class and subject to change (and being locked off under the module system).

For me, a *Impl should be the implementation of an interface, this one looks more like a LossesHelper with all its static methods (and the class should probably be final).

I did not went through the whole thing but it looks like these helpers could also be moved directly to Loss as protected methods?

The split really comes for module visibility. Losses should be publicly accessible, while LossesImpl should be module private. Some LossesImpl methods may be used by metrics. Whether we call it LossesImpl of LossesHelper is a matter of preference. The current methods in LossesImpl should not be restricted to Loss classes as metrics classes may also make use of them, therefore protected is not the right semantic, .

It feels uncomfortable to me that we plan to use the LossesImpl methods from other parts of our framework while restricting them from public use. When a system's built-ins rely on privileged capabilities that aren't available to 3rd-party code, I think it is commonly a big problem for the system's extensibility. In this case, I do see room to argue that these methods aren't "capabilities", but are just "implementation" which can safely be hidden. But given that it is important to us to reuse them for our own metrics, I lean toward thinking of them as capabilities that we should expose.

There is a tight symmetry between Losses and Metrics as many (but not all) metrics rely on the methods in Losses.
Don't think other packages will have this close of a relationship.

Is there a potential use case justifying exposing these to the public? Seeing as they are utilities needed to implement Losses/Metrics.

Agree with a rename to LossesHelper or LossesUtility to differentiate from interface implementation, however.

Personally, I'd lean toward using some of our own single-letter conventions for situations that are common in our own code, including L as the labels type.

This may be hard to follow consistently once several letters have been used e.g. 'L' might be needed for something other than label type. Seems a tad more confusing than the standard type names

Is there a potential use case justifying exposing these to the public? Seeing as they are utilities needed to implement Losses/Metrics.

Agree with a rename to LossesHelper or LossesUtility to differentiate from interface implementation, however.

Class should have Javadoc description, no?

I have changed the class name to LossesHelper.

I don't understand your comment on Class JavaDoc. This is what I have in my copy.

/**
 * These are helper methods for Losses and Metrics and will be module private when Java modularity is applied to
 * TensorFlow Java. These methods should not be used outside of the losses and metrics packages.
 */

The basic comment was put in a while a ago, and I just updated it to mention metrics.

Rendering issue I think. Looks good, thanks.

Can we remove this commented-out documentation?

OK