[MRG] FIX Add l1_ratio to set regularization with elasticnet penalty in Perceptron #18622

rickiepark · 2020-10-14T23:52:58Z

Reference Issues/PRs

Fixes #18620

What does this implement/fix? Explain your changes.

Add l1_ratio parameter in Perceptron class.
Default value is 0.15 like SGDClassifier.

Any other comments?

@NicolasHug Please check it out. :)

NicolasHug · 2020-10-15T12:27:41Z

sklearn/linear_model/_perceptron.py

@@ -148,13 +153,14 @@ class Perceptron(BaseSGDClassifier):
    https://en.wikipedia.org/wiki/Perceptron and references therein.
    """
    @_deprecate_positional_args
-    def __init__(self, *, penalty=None, alpha=0.0001, fit_intercept=True,
+    def __init__(self, *, penalty=None, alpha=0.0001, l1_ratio=0.15,


Unfortunately introducing l1_ratio=.15 might produce changes in the results of users who had set penalty='elasticnet', since it used to be hardcoded to 0 before.

We can:

set the default to 0, but that would be slightly weird since just setting penalty='elasticnet' would still be using L2. It would also be inconsistent with SGDClassifier.

set the default to 'warn' which is equivalent to 0, but indicates that the default will change from 0 to .15 in version 0.26

not care about all this and consider that this is a bugfix. In this case we can just set the default as it is: 0.15

I have a slight preference for option 3 since it's the simplest and it's likely that this won't affect many people.

Thoughts @rth @glemaitre @ogrisel @thomasjpfan?

I'm also in favor of option 3 as tend to perceive it as a bugfix.

And why 0.15 as default, not, for instance, 0.5?

I also slightly prefer option 3 and consider this a bug fix.

Agreed, seems like a bugfix. And the default should be consistent with SGDClassifier, which has 0.15, probably for no real reason.

cmarmo · 2020-10-27T11:32:34Z

Hi @rickiepark, it seems to me that there is a consensus about your implementation. Do you mind adding a "Fix" entry in the what's new? Thanks!

rickiepark · 2020-10-27T11:43:10Z

Hi @cmarmo

Ok, I'll add a Fix entry. Thanks! :)

NicolasHug

Thanks @rickiepark , looks good.

We will also need the following entry at the top of the whatsnew, under the "changed models" section:

|Fix| ::class:linear_model.Perceptron when penalty='elasticnet'

Also, it'd be nice to have a non-regression test, e.g. a small test in sklearn/linear_model/tests/test_perceptron.py that makes sure that changing l1_ratio will lead to different predictions / scores

Thanks!

NicolasHug · 2020-10-27T15:43:03Z

sklearn/linear_model/_perceptron.py

+        The Elastic Net mixing parameter, with 0 <= l1_ratio <= 1.
+        l1_ratio=0 corresponds to L2 penalty, l1_ratio=1 to L1.
+        Only used if `penalty` is 'elasticnet'.
+


Let's add .. versionadded:: 0.24 here

I just added versionadded. Thanks :)

NicolasHug · 2020-10-27T15:45:45Z

doc/whats_new/v0.24.rst

+- |Fix| Fixes bug in :class:`linear_model.Perceptron` where `__init__` method
+  doesn't have `l1_ratio` parameter. :pr:`18622` by 


I would suggest:

Suggested change

- |Fix| Fixes bug in :class:`linear_model.Perceptron` where `__init__` method

doesn't have `l1_ratio` parameter. :pr:`18622` by

- |Fix| Added the missing `l1_ratio` parameter in :class:`linear_model.Perceptron`, to be used when `penalty='elasticnet'`. This changes the default from 0 to 0.15. :pr:`18622` by

(make sure to properly break lines)

Thanks for helping me on this PR.
I updated the fix entry. :)

rickiepark · 2020-10-27T16:32:13Z

Thanks @NicolasHug

I'll update this pr with a test.

amueller · 2020-10-28T19:47:31Z

sklearn/linear_model/tests/test_perceptron.py

@@ -67,3 +67,13 @@ def test_undefined_methods():
    clf = Perceptron(max_iter=100)
    for meth in ("predict_proba", "predict_log_proba"):
        assert_raises(AttributeError, lambda x: getattr(clf, x), meth)
+
+
+def test_perceptron_l1_ratio():


can we get a better test?

Also, I feel like the perceptron documentation could probably be improved. Our perceptron class doesn't really implement only a perceptron. Has anyone else implemented something like this?

Not that I'm an example to follow on this, but let's try to be more descriptive in our comments ;) What kind of test would you like to see @amueller?

NicolasHug

Thanks @rickiepark , LGTM

NicolasHug · 2020-10-28T21:49:21Z

sklearn/linear_model/tests/test_perceptron.py

+    clf2 = Perceptron(l1_ratio=0.15, penalty='elasticnet')
+    clf2.fit(X, y)
+
+    assert clf1.score(X, y) != clf2.score(X, y)


It would be better to assert that the scores are different with a significant difference (e.g. something like .1 or .05).

assert clf1.score(X, y) != clf2.score(X, y) would pass even if the score difference was 1e-15 and that's not strict enough

clf1.score(X, y) is 0.96 and clf2.score(X, y) is 0.906.
How about assert clf1.score(X, y) - clf2.score(X, y) > 0.01 ?

that sounds fine. Maybe check the absolute difference?

clf1 is overfitted more than clf2. Is it normal that clf2.score(X, y) is greater than clf1.score(X, y)?

Would it make sense to have l1_ratio=0 and check with penalty="l2" and l1_ratio=1 and check with penalty="l1"?

Hi @glemaitre ,
Do you mean something like
assert Perceptron(l1_ratio=0, penalty='elasticnet').fit(X, y).score(X, y) != Perceptron(l1_ratio=0, penalty='l2').fit(X, y).score(X, y)
and
assert Perceptron(l1_ratio=1, penalty='elasticnet').fit(X, y).score(X, y) != Perceptron(l1_ratio=1, penalty='l1').fit(X, y).score(X, y)?

I pushed a small test to check that this is passing across all platforms.
We should be able to check the coefficient in l1 and with l1_ratio=1 and l2 with l1_ratio=0

sklearn/linear_model/_perceptron.py

doc/whats_new/v0.24.rst

sklearn/linear_model/_perceptron.py

sklearn/linear_model/tests/test_perceptron.py

glemaitre · 2020-10-30T10:33:18Z

sklearn/linear_model/tests/test_perceptron.py

+    clf2 = Perceptron(l1_ratio=0.15, penalty='elasticnet')
+    clf2.fit(X, y)
+
+    assert clf1.score(X, y) != clf2.score(X, y)


Would it make sense to have l1_ratio=0 and check with penalty="l2" and l1_ratio=1 and check with penalty="l1"?

sklearn/linear_model/_perceptron.py

glemaitre

So the test is passing across all platforms. We are good at merging.

glemaitre · 2020-11-02T10:53:12Z

Thanks @rickiepark

rickiepark · 2020-11-02T12:55:27Z

Thanks @glemaitre :)

Add l1_ratio parameter in Perceptron

eef16a0

github-actions bot added the module:linear_model label Oct 14, 2020

NicolasHug reviewed Oct 15, 2020

View reviewed changes

NicolasHug added this to the 0.24 milestone Oct 15, 2020

cmarmo added the Waiting for Reviewer label Oct 27, 2020

rickiepark added 2 commits October 27, 2020 21:52

Merge branch 'master' into perceptron_l1_ratio_fix

366cb22

add a fix entry in whats_new

f84a228

NicolasHug reviewed Oct 27, 2020

View reviewed changes

cmarmo removed the Waiting for Reviewer label Oct 27, 2020

rickiepark added 3 commits October 28, 2020 15:10

update fix entry

e5af9c5

add versionadded

870630b

add test_perceptron_l1_ratio()

7d55a9b

cmarmo added the Waiting for Reviewer label Oct 28, 2020

amueller reviewed Oct 28, 2020

View reviewed changes

cmarmo removed the Waiting for Reviewer label Oct 28, 2020

NicolasHug approved these changes Oct 28, 2020

View reviewed changes

NicolasHug reviewed Oct 28, 2020

View reviewed changes

glemaitre reviewed Oct 30, 2020

View reviewed changes

glemaitre changed the title ~~[MRG] Add l1_ratio parameter in Perceptron~~ [MRG] FIX Add l1_ratio to set regularization with elasticnet penalty in Perceptron Oct 30, 2020

rickiepark added 3 commits October 30, 2020 23:19

add a entry in changed models section

f5e5265

for comment and formating

ba7ab46

resolve conficts in v0.24.rst

81254c4

glemaitre self-requested a review November 2, 2020 09:33

cmarmo reviewed Nov 2, 2020

View reviewed changes

sklearn/linear_model/_perceptron.py Outdated Show resolved Hide resolved

glemaitre added 2 commits November 2, 2020 10:50

TST add test checking l1_ratio bound behaviour

2732768

MNT stupid pylance auto-import

1535e3f

glemaitre approved these changes Nov 2, 2020

View reviewed changes

glemaitre merged commit 4880f1c into scikit-learn:master Nov 2, 2020
2 checks passed

[MRG] FIX Add l1_ratio to set regularization with elasticnet penalty in Perceptron #18622

[MRG] FIX Add l1_ratio to set regularization with elasticnet penalty in Perceptron #18622

rickiepark commented Oct 14, 2020

NicolasHug Oct 15, 2020

lorentzenchr Oct 26, 2020

lorentzenchr Oct 26, 2020

thomasjpfan Oct 28, 2020

amueller Oct 28, 2020

cmarmo commented Oct 27, 2020

rickiepark commented Oct 27, 2020

NicolasHug left a comment

NicolasHug Oct 27, 2020

rickiepark Oct 28, 2020

NicolasHug Oct 27, 2020

rickiepark Oct 28, 2020

rickiepark commented Oct 27, 2020

amueller Oct 28, 2020

NicolasHug Oct 28, 2020

NicolasHug left a comment

NicolasHug Oct 28, 2020

rickiepark Oct 29, 2020

NicolasHug Oct 29, 2020

rickiepark Oct 29, 2020

glemaitre Oct 30, 2020

rickiepark Oct 30, 2020

glemaitre Nov 2, 2020 •

edited

glemaitre Oct 30, 2020

glemaitre left a comment

glemaitre commented Nov 2, 2020

rickiepark commented Nov 2, 2020

		- \|Fix\| Fixes bug in :class:`linear_model.Perceptron` where `__init__` method
		doesn't have `l1_ratio` parameter. :pr:`18622` by

	- \|Fix\| Fixes bug in :class:`linear_model.Perceptron` where `__init__` method
	doesn't have `l1_ratio` parameter. :pr:`18622` by
	- \|Fix\| Added the missing `l1_ratio` parameter in :class:`linear_model.Perceptron`, to be used when `penalty='elasticnet'`. This changes the default from 0 to 0.15. :pr:`18622` by

[MRG] FIX Add l1_ratio to set regularization with elasticnet penalty in Perceptron #18622

[MRG] FIX Add l1_ratio to set regularization with elasticnet penalty in Perceptron #18622

Conversation

rickiepark commented Oct 14, 2020

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

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

cmarmo commented Oct 27, 2020

rickiepark commented Oct 27, 2020

NicolasHug 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

Choose a reason for hiding this comment

rickiepark commented Oct 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicolasHug 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glemaitre Nov 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glemaitre left a comment

Choose a reason for hiding this comment

glemaitre commented Nov 2, 2020

rickiepark commented Nov 2, 2020

glemaitre Nov 2, 2020 •

edited