8000 MNT clean-up deprecations for 1.7: multi_class in LogisticRegression by jeremiedbb · Pull Request #31241 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

MNT clean-up deprecations for 1.7: multi_class in LogisticRegression #31241

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
Merged
merged 12 commits into from
May 5, 2025
Merged

MNT clean-up deprecations for 1.7: multi_class in LogisticRegression #31241

merged 12 commits into from
May 5, 2025

Conversation

jeremiedbb
Copy link
Member
@jeremiedbb jeremiedbb commented Apr 22, 2025 •
edited
Loading

When cleaning-up the deprecation of the multi_class parameter, I found that we haven't been raising a warning the last 2 versions for the following situation:

from sklearn.datasets import load_iris
from sklearn.linear_model import LogisticRegression

iris = load_iris()

# multiclass problem with solver="liblinear" and multi_class left to default
LogisticRegression(solver="liblinear").fit(iris.data, iris.target)

In this setting it falls back to OVR because liblinear doesn't support multinomial. However the deprecation of multi_class implied that all multiclass problems would be fitted using the multinomial loss and that an error would be raised if a solver doesn't support it. See

From then on, the recommended 'multinomial' will always be used for `n_classes >= 3`.
Solvers that do not support 'multinomial' will raise an error.
Use `sklearn.multiclass.OneVsRestClassifier(LogisticRegression())` if you still want to use OvR.

That's an issue because doing it for 1.7 would break code that was working until now without any FutureWarning.

A pragmatic solution is to delay the deprecation clean-up and raise a warning for this specific case for 1 more release (I don't think we really need 2 here). This is what I implemented in this PR. What do you think @lorentzenchr @ogrisel ?
Do you think it's worth a new specific changelog entry ? I feel like the one in v1.5.rst is enough.

@jeremiedbb jeremiedbb added this to the 1.7 milestone Apr 22, 2025
@github-actions GitHub Actions
Copy link
github-actions bot commented Apr 22, 2025 •
edited
Loading

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 519b315. Link to the linter CI: here

@lorentzenchr
Copy link
Member

First of all, sorry for this mistake.

I would actually dare to carry out the deprecation in 1.7 and see if anybody complains. I have heard so many complaints about scikit-learn, but never about deprecations and their settlement .

@jeremiedbb
Copy link
Member Author

First of all, sorry for this mistake.

Well I think I reviewed your PR so we share it :)

I have heard so many complaints about scikit-learn, but never about deprecations and their settlement .

Maybe because we're cautious about backward compatibility and because we take care to break their code without notice 😉

I would actually dare to carry out the deprecation in 1.7 and see if anybody complains

It crossed my mind, but I think it's better to do the right thing. It could be a pain for some users while keeping the deprecated code 1 more release for us is not an major issue (it might delay some refactorings in logistic regression if planned though).
Let's see what @ogrisel and @adrinjalali think. If needed, I have a branch with the full clean-up ready 😄 .

thomasjpfan
thomasjpfan previously approved these changes Apr 24, 2025
View reviewed changes
Copy link
Member
@thomasjpfan thomasjpfan 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.

I prefer this PR's approach of raising a deprecation warning first.

Let's also include a changelog entry. One cycle for a deprecation warning is shorter than normal.

@thomasjpfan thomasjpfan dismissed their stale review April 24, 2025 16:11

We should still add to changelog so people know about it. One cycle for a deprecation warning is shorter than normal.

@jeremiedbb
Copy link
Member Author

Let's also include a changelog entry

Done

lorentzenchr
lorentzenchr reviewed May 5, 2025
View reviewed changes
Copy link
Member
@lorentzenchr lorentzenchr left a comment

Choose a reason for hiding this comment

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

@jeremiedbb It seems I forgot to update the solver table in the docstring of LogisticRegression: newton-cholesky now supports multiclass!!!
Could you update that here?

lorentzenchr
lorentzenchr approved these changes May 5, 2025
View reviewed changes
Copy link
Member
@lorentzenchr lorentzenchr left a comment

Choose a reason for hiding this comment

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

@jeremiedbb thanks

@lorentzenchr lorentzenchr merged commit f0c80e8 into scikit-learn:main May 5, 2025
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrinjalali adrinjalali adrinjalali approved these changes

@lorentzenchr lorentzenchr lorentzenchr approved these changes

@thomasjpfan thomasjpfan thomasjpfan left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.7
Development

Successfully merging this pull request may close these issues.
4 participants
@jeremiedbb @lorentzenchr @adrinjalali @thomasjpfan
0