[MRG+2] Handle numerical instability in ElasticNetCV and LassoCV #4226

raghavrv · 2015-02-09T10:10:31Z

Fixes #4224

Return a 0 array of size n_alphas when the maximum alpha is 0
Add NRT

@GaelVaroquaux does this seem like a reasonable fix?

raghavrv · 2015-02-09T10:11:36Z

Also @jamestwebber please take a look!

agramfort · 2015-02-09T15:29:26Z

can you add a test so we understand what you're fixing? thx

raghavrv · 2015-02-09T15:48:30Z

Sure! btw this is what I am trying to fix -

When the targets are uniform the Xy ( after normalizing etc ) becomes 0. alpha_max computed from the preprocessed Xy is also zero. The range of alphas computed using log10 returns all nans and this prevents the best of max_alphas computation at this line. I assumed this to be the cause of the referenced issue.

If my understanding is right, so far, another question to be answered is whether we should support uniform values ( n_classes = 1 ) for y or simply bail out with a nice error message.

agramfort · 2015-02-09T15:53:41Z

just tell me when you added the test

raghavrv · 2015-02-09T16:11:54Z

done :p

coveralls · 2015-02-09T16:22:48Z

Coverage increased (+0.0%) to 95.02% when pulling dd8890b on ragv:fix_4224 into a3283c6 on scikit-learn:master.

jamestwebber · 2015-02-09T17:33:52Z

I guess this fixes the issue, although I think I was a little off when I called out line 100 as the culprit, it was really line 101. Line 100 shouldn't generate any NaNs, so using nanmax there won't help. The problem is when zeros are fed into the log function on the next line. So that is still going to generate NaNs and cause problems.

But your changes around line 1095 sidestep that issue, as they ignore all NaNs in the results and there should always be one zero in the array to prevent an exception (taking nanargmax of an all-NaN array would fail). So that's why the test passes.

I didn't write a fix because I really just wasn't sure what the right behavior should be. It might make more sense to just bail out if the target vector is all zeros.

raghavrv · 2015-02-09T17:48:35Z

thanks for the comment.

But your changes around line 1095 sidestep that issue, as they ignore all NaNs in the results

True! Both the nanmax and nanargmin are unnecessary and are ought to be removed. I had initially tested using them and forgot to remove. Thanks for pointing it out :)

EDIT : have fixed the same!

coveralls · 2015-02-09T18:02:28Z

Coverage increased (+0.0%) to 95.02% when pulling ff35bcd on ragv:fix_4224 into a3283c6 on scikit-learn:master.

raghavrv · 2015-02-09T20:42:06Z

sklearn/linear_model/coordinate_descent.py

@@ -389,7 +392,6 @@ def enet_path(X, y, l1_ratio=0.5, eps=1e-3, n_alphas=100, alphas=None,
    if selection not in ['random', 'cyclic']:
        raise ValueError("selection should be either random or cyclic.")
    random = (selection == 'random')
-    models = []


Cleaning up unused variable

raghavrv · 2015-02-09T22:45:34Z

@agramfort Thanks for the review! Have addressed your comments... Pl. take a look when you find time...

MechCoder · 2015-02-10T07:31:17Z

sklearn/linear_model/coordinate_descent.py

@@ -69,6 +69,11 @@ def _alpha_grid(X, y, Xy=None, l1_ratio=1.0, fit_intercept=True,
    copy_X : boolean, optional, default True
        If ``True``, X will be copied; else, it may be overwritten.
    """
+    if np.unique(y).size == 1:


alpha_max is zero, only when fit_intercept is set to True (and when all n_targets) are the same. This block of code should be inside a if fit_intercept condition, IMO

See hidden diff (#4226 (diff)

Thanks for the comments! Should I revert it back to the old form?

If we hard-code this case, we could always just create a DummyRegressor and save a lot of work ;) I guess it would make the code ugly, though.
Maybe we should warn here.

If we hard-code this case, we could always just create a DummyRegressor and save a lot of work ;)

But this particular use case is quite uncommon right?

Maybe we should warn here.

I'll add a warning. Also since this is quite similar to n_classes = 1, do we support n_classes = 1 in classifiers? It was suggested as a smoke test in #406 ( under not so easy )...

raghavrv · 2015-02-11T12:28:40Z

@MechCoder @agramfort Have added the tests and reverted the checking to alpha_max. Please take a look...

agramfort · 2015-02-11T12:35:18Z

sklearn/linear_model/coordinate_descent.py

-                         num=n_alphas)[::-1]
-    return alphas
+
+    if alpha_max == 0:


if alpha_max < np.finfo(float).min:

would be cleaner.

Sure!

For my understanding, why is it preferable? :)

amueller · 2015-02-11T14:19:46Z

https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/tests/test_common.py#L149 ;)

MechCoder · 2015-02-11T14:22:35Z

@agramfort I feel that the checking of np.unique(y) to set the grid of alphas is incorrect because of the reasons, (that I mentioned in the comments)

This is valid only for the fit_intercept case or when X is centered. If X is not centered, then even if you all targets to be the same, alpha_max need not be zero.
This checking of np.unique(y) has to be done for all the cases which does not outweigh the benefits of not doing these calculations for a really rare case.

What do you think?

agramfort · 2015-02-11T14:46:43Z

ok makes sense. Fine with me.

raghavrv · 2015-02-11T17:34:06Z

@agramfort / @MechCoder done... could you please take a final look at this?

@GaelVaroquaux have reduced n_alphas to 3, tests now run in 0.16s.

@amueller Since now the check is for alpha_max <= np.finfo(float).resolution, I haven't added any warnings... Do you think it should still be added?

raghavrv · 2015-02-11T17:38:03Z

sklearn/linear_model/coordinate_descent.py

-                         num=n_alphas)[::-1]
-    return alphas
+
+    if alpha_max <= np.finfo(float).resolution:


@agramfort out of curiosity, why do we use resolution instead of 0 directly?
( Is it for variations in the representation of floating point in different architectures so that when 0 is stored as 0.0....1, that does not affect this check? )

to avoid passing a 0 regularization in CD code. It raises warnings for nothing.

GaelVaroquaux · 2015-02-11T17:41:12Z

@GaelVaroquaux have reduced n_alphas to 3, tests now run in 0.16s.

That's much better. Thanks

raghavrv · 2015-02-11T20:25:02Z

The python3 test for the multitask case in travis keeps failing 😭
What's confusing to me is that this had passed in my box under python3.4 / python2.7... 😕

Could someone help me figure this out? This is the log file . (Search for "nan inf", the log for the particular case extends upto 10 lines above the "nan inf" line ( 2735..2745 lines)... Which shows the alpha max is 0 however this_best_mse is nan causing the trouble... Also from the size of Xy it is the first test for the multitask case.

jamestwebber · 2015-02-11T20:37:35Z

It's this line here: https://github.com/ragv/scikit-learn/blob/fix_4224/sklearn/linear_model/coordinate_descent.py#L109

Using np.finfo(float).min is not a good idea, because multiplying it by anything >1 is going to result in an overflow (technically it's an underflow, but same diff).

np.finfo(float).min
Out[195]: -1.7976931348623157e+308

np.finfo(float).min * 1. * 10
Out[196]: -inf

raghavrv · 2015-02-11T20:50:57Z

yay!! the tests pass now! Thanks a lot @jamestwebber for clarifying that ! :)

MechCoder · 2015-02-12T06:07:40Z

sklearn/linear_model/tests/test_coordinate_descent.py

+    for model in models_multi_task:
+        for y_values in (0, -5, 5, 4.5, -4.5):
+            y2[:, 0].fill(y_values)
+            y2[:, 1].fill(2 * y_values)


Can you also check that the grid of alphas obtained are the same?
Also I think it is sufficient to just check for maybe two inputs for y, i.e 0 or 5.
After that you have my +1 for merge.

you'll have my +1 too

Done! Added +2. Thanks for the review! :)

raghavrv · 2015-02-12T13:27:57Z

also now that we test only for two inputs 0 and 5 the tests take ~~10ms~~ 90ms only...

[MRG+2] Handle numerical instability in ElasticNetCV

MechCoder · 2015-02-12T16:44:32Z

Thanks!

raghavrv changed the title ~~Handle numerical instability in ElasticNetCV~~ [MRG] Handle numerical instability in ElasticNetCV Feb 9, 2015

raghavrv force-pushed the fix_4224 branch from dd8890b to ff35bcd Compare February 9, 2015 17:51

raghavrv reviewed Feb 9, 2015
View reviewed changes

raghavrv force-pushed the fix_4224 branch 4 times, most recently from cd8fd48 to d03aac9 Compare February 9, 2015 22:44

MechCoder reviewed Feb 10, 2015
View reviewed changes

amueller mentioned this pull request Feb 10, 2015

local variable referenced before assignment in LassoCV and ElasticNetCV #4224

Closed

raghavrv force-pushed the fix_4224 branch from d03aac9 to b0fc4f7 Compare February 11, 2015 12:25

raghavrv force-pushed the fix_4224 branch from b0fc4f7 to 452bc12 Compare February 11, 2015 12:31

agramfort reviewed Feb 11, 2015
View reviewed changes

raghavrv force-pushed the fix_4224 branch 2 times, most recently from 34fac3b to 5c9c67f Compare February 11, 2015 13:51

raghavrv force-pushed the fix_4224 branch from 5c9c67f to a7c705f Compare February 11, 2015 13:54

FIX Return np.zeros(n_alphas) when the targets are uniform

5fb923a

raghavrv force-pushed the fix_4224 branch from a7c705f to 3200f89 Compare February 11, 2015 17:28

raghavrv reviewed Feb 11, 2015
View reviewed changes

TST Add enet test for the case of uniform targets

7926262

raghavrv force-pushed the fix_4224 branch from 3200f89 to 7926262 Compare February 11, 2015 17:39

FIX Use resolution instead of min from np.finfo(float)

3ac2da6

raghavrv force-pushed the fix_4224 branch from eba9817 to 3ac2da6 Compare February 11, 2015 20:40

MechCoder reviewed Feb 12, 2015
View reviewed changes

TST Check for equality in alphas_ when uniform targets are used.

8df2549

raghavrv changed the title ~~[MRG] Handle numerical instability in ElasticNetCV~~ [MRG+2] Handle numerical instability in ElasticNetCV Feb 12, 2015

MechCoder added a commit that referenced this pull request Feb 12, 2015

Merge pull request #4226 from ragv/fix_4224

2a946dd

[MRG+2] Handle numerical instability in ElasticNetCV

MechCoder merged commit 2a946dd into scikit-learn:master Feb 12, 2015

raghavrv deleted the fix_4224 branch February 12, 2015 17:17

raghavrv changed the title ~~[MRG+2] Handle numerical instability in ElasticNetCV~~ [MRG+2] Handle numerical instability in ElasticNetCV and LassoCV Mar 23, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG+2] Handle numerical instability in ElasticNetCV and LassoCV #4226

[MRG+2] Handle numerical instability in ElasticNetCV and LassoCV #4226

raghavrv commented Feb 9, 2015

raghavrv commented Feb 9, 2015

agramfort commented Feb 9, 2015

raghavrv commented Feb 9, 2015

agramfort commented Feb 9, 2015 via email

raghavrv commented Feb 9, 2015

coveralls commented Feb 9, 2015

jamestwebber commented Feb 9, 2015

raghavrv commented Feb 9, 2015

coveralls commented Feb 9, 2015

raghavrv Feb 9, 2015

raghavrv commented Feb 9, 2015

MechCoder Feb 10, 2015

MechCoder Feb 10, 2015

raghavrv Feb 10, 2015

amueller Feb 10, 2015

raghavrv Feb 11, 2015

raghavrv commented Feb 11, 2015

agramfort Feb 11, 2015

raghavrv Feb 11, 2015

amueller commented Feb 11, 2015

MechCoder commented Feb 11, 2015

agramfort commented Feb 11, 2015 via email

raghavrv commented Feb 11, 2015

raghavrv Feb 11, 2015

agramfort Feb 12, 2015

GaelVaroquaux commented Feb 11, 2015 via email

raghavrv commented Feb 11, 2015

jamestwebber commented Feb 11, 2015

raghavrv commented Feb 11, 2015

MechCoder Feb 12, 2015

agramfort Feb 12, 2015

raghavrv Feb 12, 2015

raghavrv commented Feb 12, 2015

MechCoder commented Feb 12, 2015

[MRG+2] Handle numerical instability in ElasticNetCV and LassoCV #4226

[MRG+2] Handle numerical instability in ElasticNetCV and LassoCV #4226

Conversation

raghavrv commented Feb 9, 2015

raghavrv commented Feb 9, 2015

agramfort commented Feb 9, 2015

raghavrv commented Feb 9, 2015

agramfort commented Feb 9, 2015 via email

raghavrv commented Feb 9, 2015

coveralls commented Feb 9, 2015

jamestwebber commented Feb 9, 2015

raghavrv commented Feb 9, 2015

coveralls commented Feb 9, 2015

Choose a reason for hiding this comment

raghavrv commented Feb 9, 2015

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

raghavrv commented Feb 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amueller commented Feb 11, 2015

MechCoder commented Feb 11, 2015

agramfort commented Feb 11, 2015 via email

raghavrv commented Feb 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GaelVaroquaux commented Feb 11, 2015 via email

raghavrv commented Feb 11, 2015

jamestwebber commented Feb 11, 2015

raghavrv commented Feb 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raghavrv commented Feb 12, 2015

MechCoder commented Feb 12, 2015