Added Huber loss #819

Sentient07 · 2017-03-21T23:19:33Z

f0k

Thanks for the PR, and sorry for the delay! There are some things to be changed, but it looks good in general.

f0k · 2017-04-04T18:38:43Z

lasagne/objectives.py

+    targets : Theano 2D tensor or 1D tensor
+        Either a vector of int giving the correct class index per data point
+        or a 2D tensor of one-hot encoding of the correct class in the same
+        layout as predictions (non-binary targets in [0, 1] do not work!)


Wasn't this meant for regression? You copied the description from a multi-class loss.

Ah! Yes, I will revert.

f0k · 2017-04-04T18:45:45Z

lasagne/objectives.py

+    .. math:: L_\delta (diff) = \frac{diff^2}{2} & \text{if  } |diff| \le \
+                                \delta
+    .. math:: L_\delta (diff) & = & \delta (|diff| - \frac{\delta}{2} ),\
+                                     &\text{else}


Haven't checked what this renders like, but shouldn't this use a single .. math:: statement? http://www.sphinx-doc.org/en/stable/ext/math.html#directive-math
It seems rendering cases with a one-sided bracket is not easily possible in Sphinx, so using two lines is a good workaround.

I used two here because there are two different expressions that this will return, based on the value of delta.

Usually you'd use a single expression with a conditional bracket, but that's not easily possible in Sphinx (http://tex.stackexchange.com/questions/122407/writing-conditional-equations-with-braces-in-sphinx).
Using two lines seems like a good alternative, but you can write two lines in a single .. math:: statement, as shown at http://www.sphinx-doc.org/en/stable/ext/math.html#directive-math. I guess that would be cleaner.

f0k · 2017-04-04T18:47:00Z

lasagne/objectives.py

+        layout as predictions (non-binary targets in [0, 1] do not work!)
+    delta : scalar, default 1
+        This delta value is defaulted to 1, for SmoothL1Loss
+        described in Fast-RCNN paper[1].


Should be [1]_, with an underscore in the end and a space before, if I recall the syntax correctly.

f0k · 2017-04-04T18:48:37Z

lasagne/objectives.py

+    Notes
+    -----
+    This is an alternative to the Least Squared loss for
+    regression problems.


least squares, or better squared error because that's what the function is called in Lasagne.

f0k · 2017-04-04T18:49:26Z

lasagne/objectives.py

+    Returns
+    -------
+    Theano 1D tensor
+        An expression for the item-wise huber loss.


Again, this was copied from a multi-class objective docstring. Copy it from the squared error instead (it should be a tensor of arbitrary dimensionality, and the element-wise loss).

f0k · 2017-04-04T18:50:30Z

lasagne/objectives.py

+    diff = targets - predictions
+    ift = 0.5 * squared_error(targets, predictions)
+    iff = delta * (abs(diff) - delta / 2.)
+    return theano.tensor.switch(abs(diff) <= delta, ift, iff).sum()


Don't sum in the end, it should return the element-wise loss.

f0k · 2017-04-04T18:51:12Z

lasagne/objectives.py

+           https://arxiv.org/pdf/1504.08083.pdf
+    """
+    predictions, targets = align_targets(predictions, targets)
+    diff = targets - predictions


Use abs_diff = abs(targets - predictions) instead. Maximizes reuse of Theano expressions, so Theano doesn't have to merge them later.

f0k · 2017-04-04T18:53:59Z

lasagne/tests/test_objectives.py

@@ -153,6 +153,35 @@ def test_binary_hinge_loss(colvect):


 @pytest.mark.parametrize('colvect', (False, True))
+def test_huber_loss(colvect):
+    from lasagne.objectives import huber_loss
+    delta = [0.5, 1.0]


You can add delta to the test parameters and add another parametrize. Looks a little cleaner.

f0k · 2017-04-04T18:56:34Z

lasagne/objectives.py

+    ----------
+    .. [1] Ross Girshick et al (2015):
+           Fast RCNN
+           https://arxiv.org/pdf/1504.08083.pdf


Can you also cite Huber, maybe as the first reference? https://en.wikipedia.org/wiki/Huber_loss#cite_note-1

f0k · 2017-04-04T18:59:45Z

Oh, and I forgot, the new function should be included in __all__ at the top of the file, and in the module docstring at the top of the file, and in docs/modules/objectives.rst.

Sentient07 · 2017-04-05T17:54:39Z

I have added a new commit that addresses all the comments to the best of my knowledge. I haven't verified myself what sphinx renders. There seems to be some problem with my browser(safari). If the sphinx isn't fine, please let me know I will change it and try to build that on another system and push one more commit.
Thanks

f0k

Thank you for the update! Some more comments (or repetitions of earlier comments). Almost there!

f0k · 2017-04-05T18:28:12Z

lasagne/objectives.py

@@ -86,6 +86,7 @@
    "aggregate",
    "binary_hinge_loss",
    "multiclass_hinge_loss",
+    "huber_loss",


Please also add it to the module docstring further above.

And to the .rst file as I mentioned in #819 (comment). Thank you!

f0k · 2017-04-05T18:28:57Z

lasagne/objectives.py

+    .. math:: 
+        L_\delta (diff) = \frac{diff^2}{2} & \text{if  } |diff| \le \ delta \\
+
+        L_\delta (diff) & = & \delta (|diff| - \frac{\delta}{2} ),\ \\


The alignment characters (&) still seem off -- does this render correctly in Sphinx?

f0k · 2017-04-05T18:30:07Z

lasagne/objectives.py

-        or a 2D tensor of one-hot encoding of the correct class in the same
-        layout as predictions (non-binary targets in [0, 1] do not work!)
+        Ground truth to which the prediction is to be compared
+        with.


Please copy the predictions and targets from squared_error. This is still not correct.

I am sorry, it's a little unclear to me. You mean to say, i should be having predictions and targets in a single line separated by , similar to squared_error?

Something like this?

Parameters ---------- predictions, targets : Theano tensors

Yes, or you can also call it a and b, since the loss is completely symmetric. Your current docstring says they should be 1D or 2D tensors, which is too strict! They can be anything -- it's a drop-in replacement for the squared error.

f0k · 2017-04-05T18:33:45Z

lasagne/objectives.py

    regression problems.

    References
    ----------
    .. [1] Ross Girshick et al (2015):
           Fast RCNN
           https://arxiv.org/pdf/1504.08083.pdf
+
+    .. [2] Huber, Peter et al (1964)


This line should have a colon : in the end. Sorry for the apparent nitpick, but note that the line break is only visible when viewing the docstring in Python, not in the Sphinx rendering. Also please add a period . after the title. Have a look at how this renders to be sure. (Last bullet point: https://github.com/Lasagne/Lasagne/blob/master/.github/PULL_REQUEST_TEMPLATE.md)

Ah, didn't see the comment about your browser problem. Do you have another browser you can try? (Using another computer seems overkill!) Otherwise I can also check out your PR and test the documentation here, but of course it's easier if you can debug and fix things directly!

f0k · 2017-04-05T18:34:49Z

lasagne/objectives.py

    """
    predictions, targets = align_targets(predictions, targets)
-    diff = targets - predictions
+    ab_diff = abs(targets - predictions)


I'd strongly prefer abs_diff.

It was a typo. I don't have anything against abs_diff, but yeah I understand the convention :)

f0k · 2017-04-05T18:35:30Z

lasagne/tests/test_objectives.py

-        l1 = huber_loss(a, b, delta[0])
-        l2 = huber_loss(a, b, delta[1])
+        l1 = huber_loss(a, b, delta)
+        l2 = huber_loss(a, b, delta)


You're computing the same thing twice now. The idea of making delta a test parameter was to remove the duplication :)

Ah! Yeah, sorry. I rushed through that PR. I will check every comment again before the next commit.

Sentient07 · 2017-05-31T20:08:23Z

I very much apologise for the delay. The two months were really hectic in seeking positions. I have addressed the comments that were left over. The sphinx now looks good in my local build. If there are anymore changes, I will make them right away!

f0k

Perfect, and thanks a lot for including the screenshot! There's just a bug in your test.

f0k · 2017-06-01T08:52:28Z

lasagne/tests/test_objectives.py

+    abs_diff = abs(x - y)
+    ift = 0.5 * abs_diff ** 2
+    iff = delta * (abs_diff - delta / 2.)
+    z = np.where(abdiff <= delta, ift, iff)


I'd merge this right away, only all your tests failed with NameError: global name 'abdiff' is not defined.

Sorry about that, I will fix it.

f0k · 2017-06-01T09:40:19Z

Looks good now, can you please squash everything into the first commit?

Addressed comments Fixed sphinx errors changed variable name

f0k requested changes Apr 4, 2017

View reviewed changes

f0k requested changes Apr 5, 2017

View reviewed changes

f0k requested changes Jun 1, 2017

View reviewed changes

f0k approved these changes Jun 1, 2017

View reviewed changes

Added Huber loss

8694951

Addressed comments Fixed sphinx errors changed variable name

Sentient07 force-pushed the huber-loss branch from 8a07835 to 8694951 Compare June 1, 2017 15:32

f0k merged commit ffc8b8a into Lasagne:master Feb 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Huber loss #819

Added Huber loss #819

Sentient07 commented Mar 21, 2017

f0k left a comment

f0k Apr 4, 2017

Sentient07 Apr 4, 2017

f0k Apr 4, 2017

Sentient07 Apr 4, 2017 •

edited

f0k Apr 4, 2017

f0k Apr 4, 2017

f0k Apr 4, 2017

f0k Apr 4, 2017

f0k Apr 4, 2017

f0k Apr 4, 2017

f0k Apr 4, 2017

f0k Apr 4, 2017

f0k commented Apr 4, 2017

Sentient07 commented Apr 5, 2017

f0k left a comment

f0k Apr 5, 2017

f0k Apr 5, 2017

f0k Apr 5, 2017

f0k Apr 5, 2017 •

edited

Sentient07 Apr 7, 2017

Sentient07 Apr 7, 2017

f0k Apr 7, 2017

f0k Apr 5, 2017

f0k Apr 5, 2017

f0k Apr 5, 2017

Sentient07 Apr 5, 2017

f0k Apr 5, 2017

Sentient07 Apr 5, 2017

Sentient07 commented May 31, 2017

f0k left a comment

f0k Jun 1, 2017

Sentient07 Jun 1, 2017

f0k commented Jun 1, 2017

Added Huber loss #819

Added Huber loss #819

Conversation

Sentient07 commented Mar 21, 2017

f0k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sentient07 Apr 4, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f0k commented Apr 4, 2017

Sentient07 commented Apr 5, 2017

f0k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f0k Apr 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sentient07 commented May 31, 2017

f0k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f0k commented Jun 1, 2017

Sentient07 Apr 4, 2017 •

edited

f0k Apr 5, 2017 •

edited