Skip to content

DOC Remove mention of deprecated multi_class in LogisticRegression #29998

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.

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

Merged
merged 2 commits into from
Oct 8, 2024

Conversation

lucyleeow
Copy link
Member

@lucyleeow lucyleeow commented Oct 3, 2024

Reference Issues/PRs

multi_class deprecated in #28703

What does this implement/fix? Explain your changes.

  • Remove mention of deprecated multi_class in LogisticRegression. Also moves the section on multiclass down, as it seems to make sense to introduce the class first.
  • Add some links to multiclass term, OvR class
  • Amend (solver) 'Table' link to be directly to the section with the table

Any other comments?

Copy link

github-actions bot commented Oct 3, 2024

✔️ Linting Passed

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

Generated for commit: 4a14346. Link to the linter CI: here

@jeremiedbb
Copy link
Member

Since multi_class was deprecated in 1.5, it will be removed only in 1.7 while the upcoming release is 1.6. Shouldn't we keep content related to multi_class for the until them ?

@glemaitre glemaitre self-requested a review October 3, 2024 13:03
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.

I'm inline with the changes since we can already advocate to use OneVsRestClassifier instead of the parameter. I find it complementary to the deprecation warning.

@@ -1000,6 +1000,8 @@ logistic regression, see also `log-linear model
| `ElasticNet` | :math:`\frac{1 - \rho}{2}\|W\|_F^2 + \rho \|W\|_{1,1}` |
+----------------+----------------------------------------------------------------------------------+

.. _Logistic_regression_solvers:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe drop the capital letter if possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, I agree its weird with the capital. I initially thought I should copy what was used as reference header for logistic regression main section:

.. _Logistic_regression:

@@ -827,6 +821,11 @@ class LogisticRegression(LinearClassifierMixin, SparseCoefMixin, BaseEstimator):
the L2 penalty. The Elastic-Net regularization is only supported by the
'saga' solver.

For :term:`multiclass` problems, only 'newton-cg', 'sag', 'saga' and 'lbfgs'
Copy link
Member

Choose a reason for hiding this comment

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

@jeremiedbb Since we are in the long summary and we have an alternative to use OvR, I would advocate from now on to use OneVsRestClassifier. It looks legit to me.

Copy link
Member

Choose a reason for hiding this comment

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

alright then :)

Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

LGTM once guillaume's comment is resolved

@glemaitre glemaitre merged commit 7566d30 into scikit-learn:main Oct 8, 2024
30 checks passed
@glemaitre
Copy link
Member

Thanks @lucyleeow

LGTM.

BenJourdan pushed a commit to gregoryschwartzman/scikit-learn that referenced this pull request Oct 11, 2024
@lucyleeow lucyleeow deleted the doc_logreg branch October 24, 2024 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants