Skip to content

FIX add support for multilabel classification in RidgeClassifier* #19869

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 32 commits into from
Oct 27, 2021

Conversation

glemaitre
Copy link
Member

@glemaitre glemaitre commented Apr 12, 2021

closes #19858
closes #19882
build on the top of #19859

This PR fixes the predict method in RidgeClassifierCV and enables support for RidgeClassifier when the target is a multilabel indicator matrix.

The common tests are using #19859. The change in this PR should be limited to ridge.py and test_ridge.py once #19859 is merged.

@glemaitre glemaitre changed the title FIX make predict returning a multilabel indicator matrix in RidgeClassifierCV FIX add support for multilabel classification in RidgeClassifier* Apr 13, 2021
@glemaitre
Copy link
Member Author

glemaitre commented Apr 13, 2021

@agramfort Do you have an idea of a good test to check that the classification is going well in RidgeClassifier? For the moment, I only create 2 identical outputs in y and check that the predictions for both outputs are identical:

https://github.com/scikit-learn/scikit-learn/pull/19869/files#diff-764002e679104e9ca7f9d338a599e7ada5bae3bfb2154474b172ee4c7233cedf

But maybe there is something better to be tested.

@glemaitre
Copy link
Member Author

OK this is ready for a review. Maybe @agramfort if you can have a look at this one.

Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

Besides LGTM

Thx @glemaitre

glemaitre and others added 2 commits August 7, 2021 11:29
Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

I’ll let @lorentzenchr or @rth do the second review. Thx @glemaitre

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

Overall LGTM, but I need to re-read this more carefully a second time.
Thanks for this PR!

@glemaitre
Copy link
Member Author

I address the comment @rth

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.

looks good overall, just a few minor comments

glemaitre and others added 2 commits October 27, 2021 14:11
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
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. Thanks @glemaitre !

@jeremiedbb jeremiedbb merged commit 90e04a5 into scikit-learn:main Oct 27, 2021
samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
…ikit-learn#19869)

Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Dec 24, 2021
…ikit-learn#19869)

Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
glemaitre added a commit that referenced this pull request Dec 25, 2021
…9869)

Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants