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

[MRG+1] Fix bug in SquaredEpsilonInsensitive loss function #6728 #6764

Merged
merged 5 commits into from May 10, 2016

Conversation

Projects
None yet
6 participants
@willwhy
Contributor

willwhy commented May 5, 2016

Reference Issue

This PR comes to fix #6728 .

What does this implement/fix? Explain your changes.

  • Condition of if statement is incorrect in linear_model.SquaredEpsilonInsensitive(loss function) when p - y > epsilon. It should be z < -epsilon where z = y - p.
  • Added tests for each Cython class on the gradient values in SGD.
@willwhy

This comment has been minimized.

Contributor

willwhy commented May 5, 2016

Travis-ci failed, and it seems not caused by this change. Does it matter? (This is my first pull request for sklearn, sorry if this is a naive question.)

@@ -319,7 +319,7 @@ cdef class SquaredEpsilonInsensitive(Regression):
z = y - p
if z > self.epsilon:
return -2 * (z - self.epsilon)
elif z < self.epsilon:
elif z < -self.epsilon:

This comment has been minimized.

@agramfort

agramfort May 5, 2016

Member

oh boy on did this happen. Any chance you can catch this with a test?

this should be documented in the what's new bug fix section.

thx

This comment has been minimized.

@willwhy

willwhy May 5, 2016

Contributor

Hi Alexandre, I will close this PR and start a new PR include commits of what's new document and this file, is that better, or just start a new PR only include what's new document.

This comment has been minimized.

@maniteja123

maniteja123 May 5, 2016

Contributor

Hi, it would be fine to add what's new entry and also a regression test if possible ( need to make up one as it is not being caught before) and push the new commits to the same branch (same PR). Hope that is the right thing to do here.

This comment has been minimized.

@willwhy

willwhy May 5, 2016

Contributor

Thank you! should be like this~

@jnothman

This comment has been minimized.

Member

jnothman commented May 5, 2016

Don't worry about the travis failure as it doesn't seem related to this change.

@willwhy willwhy changed the title from [MRG] Fix bug in SquaredEpsilonInsensitive loss function #6728 to [WIP] Fix bug in SquaredEpsilonInsensitive loss function #6728 May 5, 2016

@willwhy willwhy changed the title from [WIP] Fix bug in SquaredEpsilonInsensitive loss function #6728 to [MRG] Fix bug in SquaredEpsilonInsensitive loss function #6728 May 5, 2016

@@ -196,6 +196,10 @@ Bug fixes
:clas:`discriminant_analysis.LinearDiscriminantAnalysis` now returns
correct results. By `JPFrancoia`_
- Fixed bug in :class:`linear_model.SquaredEpsilonInsensitive`

This comment has been minimized.

@agramfort

agramfort May 7, 2016

Member

it affects the SGD classifier code. You should point to this class not SquaredEpsilonInsensitive

let us know when you added a test or if you need help.

This comment has been minimized.

@willwhy

willwhy May 7, 2016

Contributor

I made a mistake, it should be linear_model.sgd_fast.SquaredEpsilonInsensitive.

For the test part, although there are several tests for loss functions, they didn't assert the value of loss, just checked the final score.

I think we can add this test code below, which is not only for SquaredEpsilonInsensitive but for all loss functions in sgd. What do you think about this?

from sklearn.linear_model.sgd_fast import SquaredEpsilonInsensitive
#......
def test_loss_functions():
    # Test loss functions
    # Test SquaredEpsilonInsensitive
    loss = SquaredEpsilonInsensitive(0.01)
    assert_equal(loss.dloss(1, 0.5), 0.98)
    assert_equal(loss.dloss(0.5, 1), -0.98)
    assert_equal(loss.dloss(1, 1), 0.) # where value is incorrect for current version

This comment has been minimized.

@agramfort

agramfort via email May 7, 2016

Member

This comment has been minimized.

@willwhy

willwhy May 9, 2016

Contributor

I've added tests for each cython class on the gradient values.

@willwhy willwhy changed the title from [MRG] Fix bug in SquaredEpsilonInsensitive loss function #6728 to [WIP] Fix bug in SquaredEpsilonInsensitive loss function #6728 May 7, 2016

@willwhy willwhy changed the title from [WIP] Fix bug in SquaredEpsilonInsensitive loss function #6728 to [MRG] Fix bug in SquaredEpsilonInsensitive loss function #6728 May 9, 2016

def test_gradient_hinge():
# Test Hinge (hinge / perceptron)
loss = sgd_fast.Hinge(0.0)

This comment has been minimized.

@ogrisel

ogrisel May 9, 2016

Member

Could you also please add tests for sgd_fast.Hinge(1.0) which is the most common setting for SVMs?

This comment has been minimized.

@willwhy

willwhy May 9, 2016

Contributor

Tests for sgd_fast.Hinge(1.0) has been added, and thank you for your review! @ogrisel

This comment has been minimized.

@ogrisel

ogrisel May 9, 2016

Member

Thanks!

@ogrisel

This comment has been minimized.

Member

ogrisel commented May 9, 2016

Beside my comment this looks good to me. Thanks for the fix and the much needed gradient tests @geekoala!

@ogrisel ogrisel changed the title from [MRG] Fix bug in SquaredEpsilonInsensitive loss function #6728 to [MRG+1] Fix bug in SquaredEpsilonInsensitive loss function #6728 May 9, 2016

@ogrisel ogrisel added the Bug label May 9, 2016

@ogrisel ogrisel added this to the 0.18 milestone May 9, 2016

@ogrisel

This comment has been minimized.

Member

ogrisel commented May 9, 2016

where condition of if statement is incorrect when p - y > epsilon.
Affects :class:`linear_model.SGDClassifier` and :class:`linear_model.SGDRegressor`.
(`#6764 <https://github.com/scikit-learn/scikit-learn/pull/6764>`_). By `Wenhua Yang`_.

This comment has been minimized.

@maniteja123

maniteja123 May 9, 2016

Contributor

Just a minor thing. Could you please add a link to your github profile at the bottom so that the name on this line generates a hyperlink to the profile page? Thanks.

This comment has been minimized.

@willwhy

willwhy May 9, 2016

Contributor

Oh, I forgot this one, thank you @maniteja123

@@ -196,6 +196,11 @@ Bug fixes
:clas:`discriminant_analysis.LinearDiscriminantAnalysis` now returns
correct results. By `JPFrancoia`_
- Fixed bug in :class:`linear_model.sgd_fast.SquaredEpsilonInsensitive`

This comment has been minimized.

@maniteja123

maniteja123 May 9, 2016

Contributor

Also a minor nitpick, I suppose this class is not a public API and might not have a doc to be linked to. I think it might be better just to mention that in fixed bug in SqauredEpsilonInsensitive.

This comment has been minimized.

@ogrisel

ogrisel May 10, 2016

Member

yes I agree. I would reformulate as "Fixed incorrect gradient computation in class:sklearn.linear_model.SGDRegressor with loss='squared_epsilon_insensitive'".

This comment has been minimized.

@ogrisel

ogrisel May 10, 2016

Member

I will do the fix after the merge.

@maniteja123

This comment has been minimized.

Contributor

maniteja123 commented May 9, 2016

I am no expert in this, but the tests seem to cover all the code paths for the dloss functions. LGTM. Hope I didn't miss any code coverage.

@ogrisel ogrisel merged commit 7ce4f5c into scikit-learn:master May 10, 2016

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.004%) to 94.234%
Details
@@ -1159,3 +1161,117 @@ def test_large_regularization():
with np.errstate(all='raise'):
model.fit(iris.data, iris.target)
assert_array_almost_equal(model.coef_, np.zeros_like(model.coef_))
def _test_gradient_common(loss_function, cases):

This comment has been minimized.

@mblondel

mblondel May 10, 2016

Member

I think the tests would be more robust and more readable if you use finite difference:

expected = (loss_function.loss(p + epsilon, y) - loss_function.loss(p - epsilon, y)) / (2 * epsilon)

@willwhy willwhy deleted the willwhy:issue-6728 branch May 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment