[MRG] BUG fix RidgeClassifierCV to accept custom score by venkyyuvy · Pull Request #14848 · scikit-learn/scikit-learn

venkyyuvy · 2019-08-29T22:01:33Z

Fixes: #14672. Fixes #14856.

What does this implement/fix?

Added param is_clf to _BaseRidgeCV for identifying whether the problem is regression or classification.

Changes target variable transformation in .fit() based on input for cv because RidgeClassifierCV internally does the transformation.

I have included a test cases for both scenarios cv == None and cv!=None.

venkyyuvy · 2019-09-14T11:52:12Z

@amueller | @jnothman, help required.

My latest commit is failing in codecov/patch but I am not able to understand where I am making a mistake. In fact, I had already included a new test case for my code.

Expecting some guidance here.

venkyyuvy · 2019-09-25T04:30:37Z

ping @amueller

If time permits, please share your reviews

agramfort · 2019-09-26T08:29:56Z

@venkyyuvy please address @glemaitre reviews before asking for more.

venkyyuvy · 2019-09-30T10:02:34Z

Sorry for that. I thought that I had addressed the reviews from @glemaitre. Please let me know, if I had misunderstood.

venkyyuvy · 2019-10-08T09:02:03Z

Hi @agramfort,

There is no additional response from @glemaitre,
Can I request for another review now?

jeromedockes · 2019-10-11T13:31:24Z

the error persists when cv != None

venkyyuvy · 2019-10-11T14:38:11Z

the error persists when cv != None

True. I had raised another issue for this and am waiting for members to give me a thumbs up to start working on that.

venkyyuvy · 2019-12-06T16:11:54Z

@NicolasHug,

Could please review this PR?

glemaitre · 2019-12-06T16:18:58Z

@venkyyuvy I made a PR there: venkyyuvy#3
such that it will be ok with the refactoring done in #15652

venkyyuvy · 2019-12-07T16:11:59Z

@venkyyuvy I made a PR there: venkyyuvy#3
such that it will be ok with the refactoring done in #15652

Thanks a lot @glemaitre.
I haven't been notified of your PR before, that is the reason why I had pinged here.
Looking forward for your review now.

glemaitre

LGTM.

@qinhanmin2014 could you have a look at it. This PR is only here to check that we can pass a scoring to RidgeClassificationCV. We are getting closer to what we want to do :)

glemaitre · 2019-12-10T08:17:19Z

Feel free to push a new description

…

On Tue, 10 Dec 2019 at 09:16, Venkatachalam N ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In doc/whats_new/v0.23.rst <#14848 (comment)> : > @@ -73,6 +73,11 @@ Changelog `store_cv_values` is `True`. :pr:`15652` by :user:`Jérôme Dockès <jeromedockes>`. +- |Fix| Fixed a bug in :class:`linear_model.RidgeRegressionCV` to pass a Shouldn't this be RidgeClassifierCV? Also I had fixed a bug, which raises error when RidgeClassifierCV is fed with classifier scoring and CV!=None — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14848?email_source=notifications&email_token=ABY32P5IMBG3BSAQVLTFMXDQX5F5DA5CNFSM4ISHHMP2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCOTBDZI#pullrequestreview-329650661>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABY32P4VZ34LP6HKJWH35ALQX5F5DANCNFSM4ISHHMPQ> .

-- Guillaume Lemaitre Scikit-learn @ Inria Foundation https://glemaitre.github.io/

glemaitre · 2019-12-23T22:59:51Z

ping @amueller (maybe :))

agramfort · 2020-01-05T15:34:55Z


+@pytest.mark.parametrize("scoring", [None, "accuracy", _accuracy_callable])
+@pytest.mark.parametrize("cv", [None, KFold(5)])
+@pytest.mark.parametrize("filter_", [DENSE_FILTER, SPARSE_FILTER])


I would test also with a dummy callable that return let's say 0.42 all the time and check that the score I obtain is actually 0.42 and that the C selected is the smallest one (strongest regularization). In case of tie between cv_scores you should always pick the most regularized model.

so larger alpha then

Actually, tests are failing. We pick up the smallest alpha instead of the largest one.

The reason is that we loop over the alphas in the order in the constructor.
Therefore, it will keep alphas[0] in all cases. If we want to apply your suggestion we need to change the behaviour of RidgeCV and RidgeClassifierCV

We can address this in another PR then.

+1 to do it in another PR

Can I work this PR?

I already open one there: #16029
But feel free to comment and contribute. I think that I found some other bugs.

glemaitre · 2020-01-06T14:17:16Z

@agramfort you can probably merge then :)

agramfort · 2020-01-06T20:35:38Z

thx @venkyyuvy and @glemaitre !

…14848) * handle_clf_ridgeGCV * included test case * undo_lambda_rm * crtd_commet * check_y * include_comp_proba * fix_format * added more comment for is_clf condtn * adding param is_clf * adding super * format fixes * init params * moving param 2 inint * using is_classifier * removing super * ENH do not allocate local arrays in Ridge*CV of store_cv_vales is False * add whats new * TST check multiple metrics for RidgeCV * linting * update whats new * missing doc when solving conflict * missing doc * style * correct whats new * fix * address agramfort comments * fix Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

Venkatachalam N added 3 commits August 30, 2019 02:45

handle_clf_ridgeGCV

0f1d1ef

included test case

494f1e0

undo_lambda_rm

4b103f5

venkyyuvy changed the title ~~Scoring clf looe~~ [MRG] RidgeClassifierCV Scoring looe Aug 29, 2019

Venkatachalam N added 4 commits August 30, 2019 03:43

crtd_commet

2eb63b9

check_y

f06a0ed

include_comp_proba

fd145bb

fix_format

8ba98de

venkyyuvy changed the title ~~[MRG] RidgeClassifierCV Scoring looe~~ [MRG] RidgeClassifierCV Scoring loo Aug 31, 2019

glemaitre reviewed Sep 17, 2019

View reviewed changes

Comment thread sklearn/linear_model/ridge.py Outdated

glemaitre reviewed Sep 17, 2019

View reviewed changes

Comment thread sklearn/linear_model/ridge.py Outdated

glemaitre reviewed Sep 17, 2019

View reviewed changes

Comment thread sklearn/linear_model/ridge.py Outdated

glemaitre self-requested a review September 17, 2019 15:11

venkyyuvy force-pushed the scoring_clf_looe branch from cf381c6 to 8ba98de Compare September 23, 2019 04:39

revert_changes

112eca3

venkyyuvy closed this Sep 30, 2019

venkyyuvy reopened this Sep 30, 2019

venkyyuvy added 2 commits October 8, 2019 14:26

added more comment for is_clf condtn

3659346

Merge remote-tracking branch 'upstream/master' into scoring_clf_looe

81a0cbf

venkyyuvy added 4 commits October 13, 2019 11:17

adding param is_clf

c6ee0e3

adding super

84113ec

Merge remote-tracking branch 'upstream/master' into scoring_clf_looe

38dad51

format fixes

4df1056

glemaitre mentioned this pull request Dec 6, 2019

ENH do not allocate local arrays in Ridge*CV of store_cv_values is False #15652

Merged

Merge branch 'glemaitre-scoring_clf_looe' into scoring_clf_looe

7f490f1

glemaitre added 5 commits December 9, 2019 15:56

Merge remote-tracking branch 'origin/master' into pr/venkyyuvy/14848

e60baba

update whats new

53cb550

missing doc when solving conflict

c2c0150

missing doc

717e5a0

style

709dc10

glemaitre approved these changes Dec 9, 2019

View reviewed changes

venkyyuvy commented Dec 10, 2019

View reviewed changes

Comment thread doc/whats_new/v0.23.rst Outdated

glemaitre added 2 commits December 10, 2019 11:20

correct whats new

d7d2beb

fix

ca55e3e

ogrisel reviewed Dec 10, 2019

View reviewed changes

Comment thread sklearn/linear_model/_ridge.py

glemaitre added 2 commits December 11, 2019 11:46

Merge remote-tracking branch 'origin/master' into pr/venkyyuvy/14848

12a5654

Merge branch 'master' into scoring_clf_looe

5936b51

glemaitre changed the title ~~[MRG] RidgeClassifierCV Scoring loo~~ [MRG] BUG fix RidgeClassifierCV to accept custom score Dec 23, 2019

agramfort reviewed Jan 5, 2020

View reviewed changes

glemaitre reviewed Jan 6, 2020

View reviewed changes

Comment thread sklearn/linear_model/tests/test_ridge.py Outdated

glemaitre self-assigned this Jan 6, 2020

glemaitre added 2 commits January 6, 2020 12:07

address agramfort comments

004a250

fix

b11f557

glemaitre mentioned this pull request Jan 6, 2020

[WIP] BUG returned most regularized model with Ridge*CV in case of tie #16029

Open

agramfort merged commit 730d1e7 into scikit-learn:master Jan 6, 2020

Uh oh!

Uh oh!

Conversation

venkyyuvy commented Aug 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixes: #14672. Fixes #14856.

What does this implement/fix?

Uh oh!

venkyyuvy commented Sep 14, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

venkyyuvy commented Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agramfort commented Sep 26, 2019

Uh oh!

venkyyuvy commented Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

venkyyuvy commented Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeromedockes commented Oct 11, 2019

Uh oh!

venkyyuvy commented Oct 11, 2019

Uh oh!

venkyyuvy commented Dec 6, 2019

Uh oh!

glemaitre commented Dec 6, 2019

Uh oh!

venkyyuvy commented Dec 7, 2019

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

glemaitre commented Dec 10, 2019 via email

Uh oh!

Uh oh!

glemaitre commented Dec 23, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

glemaitre commented Jan 6, 2020

Uh oh!

agramfort commented Jan 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

venkyyuvy commented Aug 29, 2019 •

edited

Loading

venkyyuvy commented Sep 25, 2019 •

edited

Loading

venkyyuvy commented Sep 30, 2019 •

edited

Loading

venkyyuvy commented Oct 8, 2019 •

edited

Loading