[MRG] Add elastic net penalty to LogisticRegression #11646

NicolasHug · 2018-07-20T23:28:32Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

This PR allows the use of penalty='elastic-net' for the LogisticRegression class. Elastic net is already available in the saga solver, it's just not exposed yet.

Any other comments?

I set the l1_ratio parameter to 0.5 by default, because that's the default in ElasticNet. Should I set it to None? This would allow to raise an error if it's set to something else while penalty != 'elastic-net'.
EDIT: I set it to None and issue warning if penalty != elastic-net

TODO

amueller · 2018-07-21T04:57:36Z

So you want to set it to None but have the behavior be 0.5? I don't have a strong feeling about it.

Otherwise looks like it's going in the right direction :)

It seems to be working for binary classification and for multiclass when multi_class='ovr'. I'm having a hard time figuring out the intricacies of multi_class='multinomial'.

NicolasHug · 2018-07-25T15:18:54Z

I just committed a first shot at LogisticRegressionCV.

I looks OK for binary classification and multiclass when multiclass='ovr', but does not work for multiclass='multinomial'.

I'm pretty sure that the problem comes from this line, which was fine when there was only one parameter to optimize (C), but now it messes up with the format of the scores array. So the computation of the best_index (index of the best parameters), which depends on the score array, is incorrect.

I tried various ways of dealing with this but every single change I tried would break other parts of the code.

I'll focus on the doc and the examples for now, but any help / insight would be helpful :)

added warning message is user sets l1_ratio while penalty is not elastic-net

…into elastic_net_linear_classifier

NicolasHug · 2018-07-30T18:13:51Z

One of the doctests in 3.6 if failing simply because of a different formatting for floats in numpy arrays... Is there any clean way to ignore it? I guess changing the expected output string would break the other versions tests...

jnothman · 2018-07-31T09:27:25Z

we only do those doctests with the latest numpy, so you should conform

…into elastic_net_linear_classifier

Also added some comments that are hopefully clear. Still need to fix refit=False

NicolasHug · 2018-08-02T20:39:35Z

Marking this as MRG for a first review, looks like LogisticRegressionCV is finally working as expected \o/

…into elastic_net_linear_classifier

jnothman · 2018-08-05T23:38:29Z

please ping after the upcoming release

…ribute

…/scikit-learn into elastic_net_linear_classifier

NicolasHug · 2018-11-15T15:36:52Z

doc/whats_new/v0.21.rst

+  with the 'saga' solver. :issue:`11646` by :user:`Nicolas Hug <NicolasHug>`.
+
+- |Fix| Fixed a bug in the 'saga' solver where the weights would not be
+  correctly updated in some cases :issue:`11646` by `Tom Dupre la Tour`_.


Thanks for the review @TomDLT , please feel free to modify this entry for a more accurate description. I don't understand the bug fix in details.

Add a . after "some cases"

TomDLT

Thanks !

amueller · 2018-11-15T20:52:40Z

doc/modules/linear_model.rst

@@ -799,8 +813,8 @@ In a nutshell, the following table summarizes the penalties supported by each so
 | Robust to unscaled datasets  |       yes       |     yes     |       yes       |    no     |    no      |
 +------------------------------+-----------------+-------------+-----------------+-----------+------------+

-The "saga" solver is often the best choice but requires scaling. The "liblinear" solver is
-used by default for historical reasons.
+The "saga" solver is often the best choice but requires scaling. The


Unrelated but we're moving the default to lbfgs and there is no mention of this in the summary. That's a different PR though.

amueller · 2018-11-15T20:53:48Z

doc/modules/linear_model.rst

-"multinomial", an optimal C is obtained by minimizing the cross-entropy
-loss.
+multiclass case, if `multi_class` option is set to "ovr", an optimal C and
+l1_ratio is obtained for each class and if the `multi_class` option is set to


Wait, we're not using accuracy here or scoring? That seems odd. Is that wording really correct?

LogisticRegressionCV selects best parameters according to its scoring parameter.
The default scoring option used is 'accuracy'.

We should clarify the doc.

@NicolasHug can you please fix this, even though it wasn't your error?

amueller · 2018-11-15T21:25:43Z

sklearn/linear_model/logistic.py

+    it can be changed using the cv parameter. In the case of newton-cg and
+    lbfgs solvers, we warm start along the path i.e guess the initial
+    coefficients of the present fit to be the coefficients got after
+    convergence in the previous fit, so it is supposed to be faster for


The language here is not that great but not your fault...

jnothman

F438

A nitpick...

jnothman · 2018-11-18T05:16:08Z

doc/whats_new/v0.21.rst

+  with the 'saga' solver. :issue:`11646` by :user:`Nicolas Hug <NicolasHug>`.
+
+- |Fix| Fixed a bug in the 'saga' solver where the weights would not be
+  correctly updated in some cases :issue:`11646` by `Tom Dupre la Tour`_.


Add a . after "some cases"

amueller

looks good apart from nitpicks.

amueller · 2018-11-19T20:39:33Z

sklearn/linear_model/tests/test_logistic.py

+@pytest.mark.parametrize('C', np.logspace(-3, 2, 4))
+@pytest.mark.parametrize('l1_ratio', [.1, .5, .9])
+def test_elastic_net_versus_sgd(C, l1_ratio):
+    # Compare elasticnet penatly in LogisticRegression() and SGD(loss='log')


Sorry yeah, will do it tomorrow!

amueller · 2018-11-19T20:41:11Z

sklearn/linear_model/tests/test_logistic.py

+                                cv=n_folds, l1_ratios=l1_ratios,
+                                random_state=0)
+    lrcv.fit(X, y)
+    coefs_paths = np.asarray(list(lrcv.coefs_paths_.values()))


if l1_ratios is not specified, do we squeeze? Otherwise this would be a backward-incompatible change, right?

not fixed yet

We don't need to squeeze because if l1_ratios is None then we just don't even add the new dimension (see here).

The existing test that check the shapes of the attributes is left unchanged (test_logistic_cv)

amueller · 2018-11-19T20:44:11Z

sklearn/linear_model/tests/test_logistic.py

+    # make sure LogisticRegressionCV gives same best params (l1 and C) as
+    # GridSearchCV when penalty is elasticnet
+
+    if multi_class == 'ovr':


yes, multiclass ovr is treated below in test_LogisticRegressionCV_GridSearchCV_elastic_net_ovr, just added a comment

amueller · 2018-11-19T20:46:22Z

sklearn/linear_model/logistic.py

+        # _log_reg_scoring_path will output different shapes depending on the
+        # multi_class param, so we need to reshape the outputs accordingly.
+        # After reshaping,
+        # - scores is of shape (n_classes X n_folds X n_Cs . n_l1_ratios)


What's the shape of Cs and why do we always have to take C[0]?

Cs is of shape (n_classes . n_folds . n_l1_ratios, n_Cs) and all the rows are equal, so we just take the first one.

I added a comment.

amueller · 2018-11-19T20:47:26Z

sklearn/linear_model/logistic.py

+            )
+            # equiv to coefs_paths = np.moveaxis(coefs_paths, (0, 1, 2, 3),
+            #                                                 (1, 2, 0, 3))
+            coefs_paths = np.swapaxes(coefs_paths, 0, 1)


you find two swapaxes easier to read than one rollaxis? I don't actually have an opinion as the comment above makes it pretty clear.

I did at the time when I was trying to break it down but not anymore ^^

amueller · 2018-11-20T15:34:25Z

test failing...

NicolasHug · 2018-11-20T15:56:25Z

Aaah... moveaxis isn't supported in old numpy version, that's actually why I needed the weird swapaxes.

Sorry

amueller · 2018-11-22T01:23:51Z

thanks!

GaelVaroquaux · 2018-11-22T09:57:11Z

Hurray!!

TomDLT · 2018-11-22T10:03:09Z

Great ! 🎊

jnothman · 2018-11-25T21:50:18Z

Exciting!

* First draft on elasticnet penaly for LogisticRegression * Some basic tests * Doc update * First draft for LogisticRegressionCV. It seems to be working for binary classification and for multiclass when multi_class='ovr'. I'm having a hard time figuring out the intricacies of multi_class='multinomial'. * Changed default to None for l1_ratio. added warning message is user sets l1_ratio while penalty is not elastic-net * Some more doc * Updated example to plot elastic net sparsity * Fixed flake8 * Fixed test by not modifying attribute in fit * Fixed doc issues * WIP * Partially fixed logistic_reg_CV for multinomial. Also added some comments that are hopefully clear. Still need to fix refit=False * Fixed doc issue * WIP * Fixed test for refit=False in LogisticRegressionCV * Fixed Python 2 numpy version issue * minor doc updates * Weird doc error... * Added test to ensure that elastic net is at least as good as L1 or L2 once l1_ratio has been optimized with grid search Also addressed minor reviews * Fixed test * addressed comments * Added back ignore warning on tests * Added a functional test * Scale data in test... Now failing * elastic-net --> elasticnet * Updated doc for some attributes and checked their shape in tests * Added l1_ratio dimension to coefs_paths and scores attr * improve example + fix test * FIX incorrect lagged_update in SAGA * Add non-regression test for SAGA's bug * FIX flake8 and warning * Re fixed warning * Updated some tests * Addressed comments * more comments and added dimension to LogisticRegressionCV.n_iter_ attribute * Updated whatsnew for 0.21 * better doc shape looks * Fixed whatnew entry after merges * Added dot * Addressed comments + standardized optional default param docstrings * Addessed comments * use swapaxes instead of unsupported moveaxis (hopefully fixes tests)

…earn#11646)" This reverts commit ff2b430.

* First draft on elasticnet penaly for LogisticRegression * Some basic tests * Doc update * First draft for LogisticRegressionCV. It seems to be working for binary classification and for multiclass when multi_class='ovr'. I'm having a hard time figuring out the intricacies of multi_class='multinomial'. * Changed default to None for l1_ratio. added warning message is user sets l1_ratio while penalty is not elastic-net * Some more doc * Updated example to plot elastic net sparsity * Fixed flake8 * Fixed test by not modifying attribute in fit * Fixed doc issues * WIP * Partially fixed logistic_reg_CV for multinomial. Also added some comments that are hopefully clear. Still need to fix refit=False * Fixed doc issue * WIP * Fixed test for refit=False in LogisticRegressionCV * Fixed Python 2 numpy version issue * minor doc updates * Weird doc error... * Added test to ensure that elastic net is at least as good as L1 or L2 once l1_ratio has been optimized with grid search Also addressed minor reviews * Fixed test * addressed comments * Added back ignore warning on tests * Added a functional test * Scale data in test... Now failing * elastic-net --> elasticnet * Updated doc for some attributes and checked their shape in tests * Added l1_ratio dimension to coefs_paths and scores attr * improve example + fix test * FIX incorrect lagged_update in SAGA * Add non-regression test for SAGA's bug * FIX flake8 and warning * Re fixed warning * Updated some tests * Addressed comments * more comments and added dimension to LogisticRegressionCV.n_iter_ attribute * Updated whatsnew for 0.21 * better doc shape looks * Fixed whatnew entry after merges * Added dot * Addressed comments + standardized optional default param docstrings * Addessed comments * use swapaxes instead of unsupported moveaxis (hopefully fixes tests)

NicolasHug added 3 commits July 20, 2018 19:15

First draft on elasticnet penaly for LogisticRegression

42453f2

Some basic tests

b63275d

Doc update

12e1d3c

NicolasHug mentioned this pull request Jul 23, 2018

Doc improvement suggestion for ElasticNetCV #11659

Closed

First draft for LogisticRegressionCV.

5cb41c4

It seems to be working for binary classification and for multiclass when multi_class='ovr'. I'm having a hard time figuring out the intricacies of multi_class='multinomial'.

NicolasHug added 7 commits July 25, 2018 17:08

Changed default to None for l1_ratio.

60948e0

added warning message is user sets l1_ratio while penalty is not elastic-net

Some more doc

318d389

Updated example to plot elastic net sparsity

202f900

Fixed flake8

1292b66

Fixed test by not modifying attribute in fit

7598d43

Fixed doc issues

080b831

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

3aaaea5

…into elastic_net_linear_classifier

NicolasHug mentioned this pull request Aug 1, 2018

[MRG] Fixed bug in _log_reg_scoring_path for multinomial case #11724

Merged

NicolasHug added 9 commits August 2, 2018 10:26

WIP

7481acd

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

4ce00c0

…into elastic_net_linear_classifier

Partially fixed logistic_reg_CV for multinomial.

43ef047

Also added some comments that are hopefully clear. Still need to fix refit=False

Fixed doc issue

6caf75d

WIP

1326d3d

Fixed test for refit=False in LogisticRegressionCV

d3b215f

Fixed Python 2 numpy version issue

d79d358

minor doc updates

0d505c3

Weird doc error...

71872b3

NicolasHug changed the title ~~[WIP] Add elastic net penalty to LogisticRegression~~ [MRG] Add elastic net penalty to LogisticRegression Aug 2, 2018

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

0af6d35

…into elastic_net_linear_classifier

NicolasHug added 6 commits November 15, 2018 09:47

more comments and added dimension to LogisticRegressionCV.n_iter_ att…

f672fa9

…ribute

Updated whatsnew for 0.21

3396ab1

better doc shape looks

ccf330d

Merge branch 'elastic_net_linear_classifier' of github.com:NicolasHug…

6e5a2c0

…/scikit-learn into elastic_net_linear_classifier

Merge branch 'master' into elastic_net_linear_classifier

0a4f156

Fixed whatnew entry after merges

b194d93

NicolasHug commented Nov 15, 2018

View reviewed changes

TomDLT approved these changes Nov 15, 2018

View reviewed changes

amueller reviewed Nov 15, 2018

View reviewed changes

jnothman reviewed Nov 18, 2018

View reviewed changes

Added dot

85e2d14

amueller mentioned this pull request Nov 19, 2018

Update LogisticRegression table summary to mention l-bfgs default #12617

Closed

amueller reviewed Nov 19, 2018

View reviewed changes

NicolasHug added 2 commits November 19, 2018 16:36

Addressed comments + standardized optional default param docstrings

a2188d6

Addessed comments

f7ff641

use swapaxes instead of unsupported moveaxis (hopefully fixes tests)

f124c88

Merge branch 'master' into elastic_net_linear_classifier

59094d6

amueller merged commit c1f5874 into scikit-learn:master Nov 22, 2018

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019

Revert "[MRG] Add elastic net penalty to LogisticRegression (scikit-l…

434a479

…earn#11646)" This reverts commit ff2b430.

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019

Revert "[MRG] Add elastic net penalty to LogisticRegression (scikit-l…

0969b02

…earn#11646)" This reverts commit ff2b430.

Uh oh!

[MRG] Add elastic net penalty to LogisticRegression #11646

[MRG] Add elastic net penalty to LogisticRegression #11646

Uh oh!

Conversation

Uh oh!

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

TODO

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!

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!

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!

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!

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!

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!

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!

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!

Uh oh!

Uh oh!

Uh oh!