[go: nahoru, domu]
Skip to content
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.

Sign up for GitHub

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

Jump to bottom

API deprecate penalty='none' for LogisticRegression #23877

Merged
merged 21 commits into from
Jul 27, 2022
Merged

API deprecate penalty='none' for LogisticRegression #23877

merged 21 commits into from
Jul 27, 2022

Conversation

MaxwellLZH
Copy link
Contributor

Reference Issues/PRs

Fixes #23749.

What does this implement/fix? Explain your changes.

This PR deprecates penalty='none' for LogisticRegression, so the usage of penalty=None is consistent among SGDClassifier, SGDRegressor, Perceptron and LogisticRegression.

@MaxwellLZH MaxwellLZH marked this pull request as draft July 10, 2022 15:00
@MaxwellLZH MaxwellLZH marked this pull request as ready for review July 11, 2022 01:59
TomDLT
TomDLT approved these changes Jul 13, 2022
View reviewed changes
Copy link
Member
@TomDLT TomDLT left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! You also need a changelog entry in doc/whats_new/v1.2.rst.

sklearn/linear_model/_logistic.py Outdated Show resolved Hide resolved
sklearn/linear_model/_logistic.py Outdated Show resolved Hide resolved
sklearn/linear_model/_logistic.py Outdated Show resolved Hide resolved
sklearn/linear_model/tests/test_logistic.py Outdated Show resolved Hide resolved
sklearn/linear_model/tests/test_logistic.py Outdated Show resolved Hide resolved
sklearn/linear_model/_logistic.py Outdated Show resolved Hide resolved
MaxwellLZH and others added 10 commits July 18, 2022 21:39
@MaxwellLZH @TomDLT
change remove version to 1.4
dc9e355 
Co-authored-by: Tom Dupré la Tour <tom.dupre-la-tour@m4x.org>
@MaxwellLZH @TomDLT
change remove version to 1.4
50cd201 
Co-authored-by: Tom Dupré la Tour <tom.dupre-la-tour@m4x.org>
@MaxwellLZH @TomDLT
change remove version to 1.4
7d15fa7 
Co-authored-by: Tom Dupré la Tour <tom.dupre-la-tour@m4x.org>
@MaxwellLZH @TomDLT
change remove version to 1.4
a378ea2 
Co-authored-by: Tom Dupré la Tour <tom.dupre-la-tour@m4x.org>
@MaxwellLZH @TomDLT
change remove version to 1.4
2377332 
Co-authored-by: Tom Dupré la Tour <tom.dupre-la-tour@m4x.org>
@MaxwellLZH @TomDLT
deprecate string option none
b8c1ae5 
Co-authored-by: Tom Dupré la Tour <tom.dupre-la-tour@m4x.org>
@MaxwellLZH
update whatsnew
b96dcdc
@MaxwellLZH
Merge branch 'api/lr-penalty' of github.com:MaxwellLZH/scikit-learn i…
8b7c360 
…nto api/lr-penalty
@MaxwellLZH
add none to param validation
f7cbae9
@MaxwellLZH
black formatting
54ef845
TomDLT
TomDLT approved these changes Jul 18, 2022
View reviewed changes
doc/whats_new/v1.2.rst Outdated Show resolved Hide resolved
doc/whats_new/v1.2.rst Outdated Show resolved Hide resolved
sklearn/linear_model/_logistic.py Outdated Show resolved Hide resolved
MaxwellLZH and others added 3 commits July 19, 2022 09:16
@MaxwellLZH @TomDLT
fix typo in whatsnew
4e59c37 
Co-authored-by: Tom Dupré la Tour <tom.dupre-la-tour@m4x.org>
@MaxwellLZH @TomDLT
fix comment string
9fbc98d 
Co-authored-by: Tom Dupré la Tour <tom.dupre-la-tour@m4x.org>
@MaxwellLZH @TomDLT
fix typo in whatsnew
98a142a 
Co-authored-by: Tom Dupré la Tour <tom.dupre-la-tour@m4x.org>
thomasjpfan
thomasjpfan approved these changes Jul 19, 2022
View reviewed changes
Copy link
Member
@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codewise, this LGTM.

As for deprecating "none", I prefer to wait for another maintainer's opinion before merging. I feel like deprecating and removing "none" will impact a lot of users. A more conservative option would be to support both options.

Overall, I am +1 with the deprecation.

sklearn/linear_model/_logistic.py Show resolved Hide resolved
sklearn/linear_model/_logistic.py Show resolved Hide resolved
sklearn/linear_model/_logistic.py Outdated Show resolved Hide resolved
MaxwellLZH and others added 3 commits July 20, 2022 09:20
@MaxwellLZH @thomasjpfan
Add TODO
77370ff 
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
@MaxwellLZH @thomasjpfan
Add TODO
e183876 
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
@MaxwellLZH @thomasjpfan
update todo doc
bee3f0f 
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
@TomDLT
Copy link
Member
TomDLT commented Jul 20, 2022

I feel like deprecating and removing "none" will impact a lot of users. A more conservative option would be to support both options.

I agree we could avoid a deprecation cycle by supporting both "none" and None.
The downside is that it might lead users to assume "none" is an acceptable option for other cases such as SGD(penalty="none").

No strong opinion, let's say we accept both options in this case, to avoid messing with existing code calling LogisticRegression(penalty="none").

glemaitre
glemaitre approved these changes Jul 27, 2022
View reviewed changes
Copy link
Member
@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A deprecation cycle is fine IMO.

@glemaitre glemaitre merged commit 3a9e708 into scikit-learn:main Jul 27, 2022
@glemaitre
Copy link
Member

Thanks @MaxwellLZH

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

@thomasjpfan thomasjpfan thomasjpfan approved these changes

@glemaitre glemaitre glemaitre approved these changes

@TomDLT TomDLT TomDLT approved these changes

Assignees
No one assigned
Labels
module:linear_model Waiting for Reviewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make penalty parameter consistent for None
4 participants
@MaxwellLZH @TomDLT @glemaitre @thomasjpfan