[MRG + 1] ElasticNetCV: raise ValueError if l1_ratio=0 #7591

erikcs · 2016-10-06T10:37:08Z

Reference Issue

What does this implement/fix? Explain your changes.

Before,

import numpy as np
from sklearn.linear_model import ElasticNetCV
X = np.array([[1, 2, 4, 5, 8], [3, 5, 7, 7, 8]]).T
y = np.array([12, 10, 11, 21, 5])

est = ElasticNetCV(l1_ratio=0).fit(X, y)

gives the runtime error UnboundLocalError: local variable 'best_l1_ratio' referenced before assignment

Now it returns ValueError: l1_ratio = 0 not supported

agramfort · 2016-10-07T07:43:30Z

please add a test

erikcs · 2016-10-07T10:19:03Z

Added a test to make sure ValueError is raised. Sorry about that.

lesteve · 2016-10-07T10:39:10Z

Hmmm I am wondering whether this is the right fix. If you look at the ElasticNetCV docstring, it seems to indicate that l1_ratio = 0 is supported:

For l1_ratio = 0 the penalty is an L2 penalty.

Maybe you need to understand why you end up with the error UnboundLocalError: local variable 'best_l1_ratio' referenced before assignment.

Tip: you can add syntax-highlighting to your snippet by adding py after the first three initial backquotes (I edited your message with this change).

erikcs · 2016-10-07T10:45:40Z

Yes, this is just a quick fix to disallow l1_ratio1=0 (and avoid the confusing error) as suggested by agramfort and amueller in issue #7551 (where I explain why the UnboundLocalError error is raised) (Thanks for the tip)

lesteve · 2016-10-07T11:42:29Z

Yes, this is just a quick fix to disallow l1_ratio1=0 (and avoid the confusing error) as suggested by agramfort and amueller in issue #7551

My bad I missed the associated issue somehow, maybe you want to update the docstring accordingly then.

lesteve · 2016-10-07T14:26:12Z

sklearn/linear_model/coordinate_descent.py

@@ -1063,6 +1063,8 @@ def fit(self, X, y):

        if hasattr(self, 'l1_ratio'):
            model_str = 'ElasticNet'
+            if self.l1_ratio == 0:
+                raise ValueError("l1_ratio = 0 not supported")


Maybe you could add a mention about L2 penalty and RidgeCV like you did in the docstring?

Just curious what happens if l1_ratio is a list which contains 0?

Sorry, that was sloppy of me. If 0 was contained in the list, the code would have run without an error, but the mse_path_ on the estimator for that value, would all be nans. I.e. a user would think the ridge estimate was evaluated, when it was in fact not. I updated the error message.

amueller · 2016-10-07T16:39:55Z

sklearn/linear_model/tests/test_coordinate_descent.py

+        clf = ElasticNetCV(l1_ratio=0)
+        clfl = ElasticNetCV(l1_ratio=[0, 0.5])
+        clfm = MultiTaskElasticNetCV(l1_ratio=0)
+        assert_raises(ValueError, clf.fit, X, y)


Can you use assert_raise_message to check the error message? Apart from that LGTM.

amueller · 2016-10-07T16:57:10Z

LGTM

agramfort · 2016-10-12T13:59:04Z

solver should work but it's auto mode for alpha grid that breaks

jnothman · 2016-10-13T13:35:43Z

@agramfort To confirm, you consider this the wrong fix?

amueller · 2016-10-13T18:01:43Z

I'm confused.

agramfort · 2016-10-07T10:13:35Z

sklearn/linear_model/tests/test_coordinate_descent.py

@@ -712,3 +712,13 @@ def test_enet_float_precision():
            assert_array_almost_equal(intercept[np.float32],
                                      intercept[np.float64],
                                      decimal=4)
+
+    def test_enet_l1_ratio():


too much indented

agramfort · 2016-10-15T08:38:05Z

Estimator would work with l1_ratio=0. if use passes his own grid of alphas. The solver would manage. It's just the automatic alpha grid that breaks.

fix is good enough but maybe I would throw the error if alphas are not passed explicitly.

clear?

erikcs · 2016-10-15T08:53:07Z

Sorry for any confusion, I thought I made it clear in #7551 that it is the grid generation in _alpha_grid that fails. Is there anything I else I should do here?

agramfort · 2016-10-15T11:59:59Z

can you just make the grid generation fail. It should only be called if alphas=None

erikcs · 2016-10-15T13:17:23Z

Ok, to be sure: revert everything in this commit, and just raise a ValueError if _alpha_grid is called with l1_ratio=0? It is currently only called if alphas=None. Should I update the docstring and add a test? Is the correct procedure for me to $git delete elnet-l1ratio, create a new branch with the same name, $git branch elnet-l1-ratio, then push to it? Or do I $git reset this branch? Thanks

agramfort · 2016-10-15T14:52:10Z

Yes correct. Make sure error message is as explicit as possible

On Sat, Oct 15, 2016 at 3:17 PM +0200, "nuffe" notifications@github.com wrote:

Ok, to be sure: revert everything in this commit, and just raise a ValueError if _alpha_grid is called with l1_ratio=0? It is currently only called if alphas=None. Should I update the docstring and add a test?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

agramfort · 2016-10-17T13:10:24Z

sklearn/linear_model/tests/test_coordinate_descent.py

+    assert_raise_message(ValueError, msg,
+                         ElasticNetCV(l1_ratio=0).fit, X, y)
+    assert_raise_message(ValueError, msg,
+                         MultiTaskElasticNetCV(l1_ratio=0).fit, X, X)


test that is works with you pass some alphas

I am sorry for a possible stupid question, but this commit broke an unrelated test (Logistic regression) on AppVeyor:

====================================================================== FAIL: sklearn.linear_model.tests.test_logistic.test_logistic_regression_sample_weights ---------------------------------------------------------------------- Traceback (most recent call last): File "C:\Python27\lib\site-packages\nose\case.py", line 197, in runTest self.test(*self.arg) File "C:\Python27\lib\site-packages\sklearn\linear_model\tests\test_logistic.py", line 638, in test_logistic_regression_sample_weights assert_array_almost_equal(clf_cw.coef_, clf_sw.coef_, decimal=4) File "C:\Python27\lib\site-packages\numpy\testing\utils.py", line 842, in assert_array_almost_equal precision=decimal) File "C:\Python27\lib\site-packages\numpy\testing\utils.py", line 665, in assert_array_compare raise AssertionError(msg) AssertionError: Arrays are not almost equal to 4 decimals (mismatch 20.0%) x: array([[ 2.5404, 0. , 0. , -0.3094, -0.4925]]) y: array([[ 2.5405, 0. , 0. , -0.3094, -0.4924]]) sklearn.decomposition.tests.test_dict_learning.test_dict_learning_reconstruction_parallel: 4.1117s

but the same tests pass on my computer and on Travis, how do I proceed here?

agramfort · 2016-10-17T16:12:12Z

please another commit to restart appveyor. Just to see if bug is reproducible

erikcs · 2016-10-17T20:28:14Z

It is still sklearn.linear_model.tests.test_logistic.test_logistic_regression_sample_weights that fails. Conceptually it should be impossible for the addition of test A to cause unit test B to fail?(commit 5171932 does not mutate any external state, and the test passes on Travis)

agramfort · 2016-10-18T19:23:36Z

sklearn/linear_model/tests/test_coordinate_descent.py

+    est = MultiTaskElasticNetCV(l1_ratio=0, alphas=alphas)
+    with ignore_warnings():
+        est.fit(X, X)
+        est_desired.fit(X, X)


calling fit(X, X) is weird. How about fit(X, y[:, None]) ?

amueller · 2016-10-24T16:02:35Z

The failure means that we're unclean on fixing random states somewhere, I think. And windows can give numerically different results.

amueller · 2016-10-24T16:04:17Z

sklearn/linear_model/tests/test_coordinate_descent.py

+        est.fit(X, y)
+    assert_array_almost_equal(est.coef_, est_desired.coef_, decimal=5)
+
+    est_desired = MultiTaskElasticNetCV(l1_ratio=0.00001, alphas=alphas)


set the random state explicitly

amueller · 2016-10-24T16:06:26Z

Ideally we would explicitly pass random states everywhere in all tests. To fix the test failure, maybe fix the random states in the test that's failing. You can also add it in the test that you added.

erikcs · 2016-10-24T19:19:59Z

Ah, thank you. Wouldn't it be easier to set the random state in every single estimator to something else than None by default, instead of changing every test?

amueller · 2016-10-24T19:31:54Z

That would drastically change user code. We have our estimators be non-deterministic by default, which is a design choice. Changing that would be quite a severe change in API. It also requires us to be very explicit about randomness in our tests, which is A Good Thing (TM)

agramfort · 2016-10-25T08:07:06Z

thx @Nuffe

GaelVaroquaux · 2016-10-25T18:27:28Z

Ah, thank you. Wouldn't it be easier to set the random state in every single estimator to something else than None by default, instead of changing every test?

We have so far frowned from this, as it is against user expectations that a random object behaves in a deterministic way.

…7591) Raise ValueError if l1_ratio=0 in ElasticNetCV and alphas=None

erikcs mentioned this pull request Oct 6, 2016

_alpha_grid divide by zero when l1_ratio=0 (ElasticNetCV) #7551

Closed

lesteve reviewed Oct 7, 2016

View reviewed changes

amueller reviewed Oct 7, 2016

View reviewed changes

amueller changed the title ~~[MRG] ElasticNetCV: raise ValueError if l1_ratio=0~~ [MRG + 1] ElasticNetCV: raise ValueError if l1_ratio=0 Oct 7, 2016

RPGOne approved these changes Oct 12, 2016

View reviewed changes

agramfort reviewed Oct 15, 2016

View reviewed changes

erikcs added 3 commits October 15, 2016 17:29

Raise ValueError if l1_ratio=0

1671707

Add test

b6b78c4

Add test - fix typo

3fa201b

agramfort reviewed Oct 17, 2016

View reviewed changes

Extend test

02f0d4d

erikcs added 3 commits October 17, 2016 18:15

Blank commit to restart AppVeyor

6b81830

Same as test passing commit #3fa201b with diff commmented out

8bf6966

Recommit extended test (with warnings supressed)

5171932

erikcs added 2 commits October 17, 2016 22:33

See what happens with different assert_almost_equal function

043e6a6

Trigger build one more time

fc4f570

agramfort reviewed Oct 18, 2016

View reviewed changes

erikcs added 2 commits October 18, 2016 21:29

Replace weird expression

0122cc0

Try and trigger build one more time

2842246

amueller reviewed Oct 24, 2016

View reviewed changes

erikcs added 2 commits October 24, 2016 18:20

Add random state to test

6de9d1a

Add random state to failed test, test_logistic_regression_sample_weights

50b1b36

agramfort merged commit 0dfc9a5 into scikit-learn:master Oct 25, 2016

erikcs deleted the elnet-l1ratio branch October 25, 2016 08:16

amueller pushed a commit to amueller/scikit-learn that referenced this pull request Oct 25, 2016

[MRG + 1] ElasticNetCV: raise ValueError if l1_ratio=0 (scikit-learn#…

a5d8067

…7591) Raise ValueError if l1_ratio=0 in ElasticNetCV and alphas=None

amueller pushed a commit to amueller/scikit-learn that referenced this pull request Oct 27, 2016

[MRG + 1] ElasticNetCV: raise ValueError if l1_ratio=0 (scikit-learn#…

4ee7996

…7591) Raise ValueError if l1_ratio=0 in ElasticNetCV and alphas=None

erikcs mentioned this pull request Jan 12, 2017

Should all test cases set the random_state? #8194

Open

sergeyf pushed a commit to sergeyf/scikit-learn that referenced this pull request Feb 28, 2017

[MRG + 1] ElasticNetCV: raise ValueError if l1_ratio=0 (scikit-learn#…

638644d

…7591) Raise ValueError if l1_ratio=0 in ElasticNetCV and alphas=None

Sundrique pushed a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017

[MRG + 1] ElasticNetCV: raise ValueError if l1_ratio=0 (scikit-learn#…

7f92e95

…7591) Raise ValueError if l1_ratio=0 in ElasticNetCV and alphas=None

paulha pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017

[MRG + 1] ElasticNetCV: raise ValueError if l1_ratio=0 (scikit-learn#…

2b99392

…7591) Raise ValueError if l1_ratio=0 in ElasticNetCV and alphas=None

maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017

[MRG + 1] ElasticNetCV: raise ValueError if l1_ratio=0 (scikit-learn#…

d3cadf7

…7591) Raise ValueError if l1_ratio=0 in ElasticNetCV and alphas=None

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[MRG + 1] ElasticNetCV: raise ValueError if l1_ratio=0 #7591

[MRG + 1] ElasticNetCV: raise ValueError if l1_ratio=0 #7591

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[MRG + 1] ElasticNetCV: raise ValueError if l1_ratio=0 #7591

[MRG + 1] ElasticNetCV: raise ValueError if l1_ratio=0 #7591

Uh oh!

Conversation

Uh oh!

Reference Issue

What does this implement/fix? Explain your changes.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!