[MRG] Add deprecation warning for iid in BaseSearchCV #9103

ldirer · 2017-06-10T15:31:50Z

Reference Issue

#9085

What does this implement/fix? Explain your changes.

Deprecate iid param.

Any other comments?

I did not find references to iid in the doc/benchmark/examples.
This will emit a couple additional DeprecationWarnings when running the tests. I'm not sure it's worth removing them.

vene · 2017-06-10T16:07:09Z

sorry, what is the connection between this and #9031?

amueller · 2017-06-10T16:09:19Z

sklearn/grid_search.py

@@ -398,6 +398,11 @@ def __init__(self, estimator, scoring=None,
        self.pre_dispatch = pre_dispatch
        self.error_score = error_score

+        if not self.iid:


iid should be set to None and if it's not none, the warning should be shown, and if it is None, it should behave as True (as that's the current behavior)

ldirer · 2017-06-10T16:10:13Z

Sorry about linking to the wrong issue, failed tab completion on my side.

vene · 2017-06-10T16:27:22Z

lol, I was hoping the corresponding issue would have some links to resources that I could use to authoritatively convince others why this kind of cv is wrong. Thanks though!

GaelVaroquaux · 2017-06-10T18:58:36Z

lol, I was hoping the corresponding issue would have some links to resources that I could use to authoritatively convince others why this kind of cv is wrong. Thanks though!

The core issue is that "iid=True" does not compute what cross-validation strives to estimate (formula 3 of https://academic.oup.com/gigascience/article/doi/10.1093/gigascience/gix020/3073663/Using-and-understanding-cross-validation ) Yes, it's complicated, but that what it really boils down to.

vene · 2017-06-11T06:16:35Z

This is a great writeup, thanks @GaelVaroquaux!

…model_selection module

jnothman · 2017-06-18T12:01:16Z

sklearn/model_selection/_search.py

+        ..deprecated:: 0.19
+            Parameter ``iid`` has been deprecated in version 0.19 and
+            will be removed in 0.21.
+            Future (and default) behavior is equivalent to `iid=true`.


But isn't @GaelVaroquaux saying that this behaviour is inappropriate? #9103 (comment)

Rather we should be both deprecating the parameter and changing the default.

raghavrv · 2017-07-13T21:21:40Z

Needs a merge with master

amueller · 2017-07-15T23:11:52Z

Ok this is really bad. We want this to be False. So what we need to do, and need to do before 0.19, is to warn people that the default behavior will change.
So set the default to None, if it's None, warn and behave as True and say that the default will be False in 0.21 and it will be removed in 0.23. 😢

This will likely raise a warning for everybody using GridSearchCV.

amueller · 2017-07-16T00:05:16Z

Actually, we should detect whether score is accuracy or the score method of ClassifierMixin and not raise a warning in that case.

amueller · 2017-07-16T00:19:11Z

Hmm questions is whether we will special case the pipeline score to avoid warning in that case if possible.

amueller · 2017-07-16T17:09:15Z

wait, I thought the iid was doing something different from what it was doing. It's just doing a weighted mean instead of a non-weighted mean? So that is independent of what metric we are using.... and we need to warn whenever the sample numbers are not the same?

amueller · 2017-07-16T18:36:17Z

hm... not sure that is actually a blocker... I thought it would to actually equal treatment of all data points, but it doesn't really.

agramfort · 2017-07-16T19:19:33Z

for me one option is the same as cross_val_score(...).mean() and the other is metric(y, cross_val_predict(X, y, ...))

jnothman · 2017-07-16T23:19:11Z

well iid does neither of those, just weights the average.

…

On 17 Jul 2017 5:19 am, "Alexandre Gramfort" ***@***.***> wrote: for me one option is the same as cross_val_score(...).mean() and the other is metric(y, cross_val_predict(X, y, ...)) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#9103 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz64phydqgvb25lis0-nKMhrU0OjUAks5sOmJGgaJpZM4N2JFL> .

agramfort · 2017-07-17T06:22:55Z

ok. Yeah let's remove this weird option.

ogrisel · 2017-07-17T08:59:56Z

+1 for deprecating the iid parameter and changing the default behavior (to do the equivalent of iid=False) right away without a FutureWarning but with proper documentation in the whats_new.rst.

In practice, it's very unlikely to significantly change the outcome of a grid search as most of our CV strategies will yield CV-folds with approximately equal sizes. The impact will probably be at noise level.

jnothman · 2017-07-17T09:27:16Z

The default can be changed immediately, but not without warning. The change may be noise, but it should be explicable for users

…

On 17 Jul 2017 6:59 pm, "Olivier Grisel" ***@***.***> wrote: +1 for deprecating iid and changing the default behavior (to do the equivalent of iid=False) right away without a FutureWarning but with proper documentation in the whats_new.rst. I practice, it's very unlikely to significantly change the outcome of a grid search as most of our CV strategies will yield CV-folds with approximately equal sizes. The impact will probably be at noise level. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#9103 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz64LE4ppdZe-ZMlbwDx0rEAK-TX_Xks5sOyKNgaJpZM4N2JFL> .

ogrisel · 2017-07-17T09:52:36Z

I am afraid that the FutureWarning is going to be very annoying. It cannot be silenced by passing iid=False explicitly because this would trigger a DeprecationWarning and we don't want to encourage our users to explicit use the iid parameter in their code.

jnothman · 2017-07-17T10:19:27Z

that's a fair point.

jnothman · 2017-07-17T10:31:53Z

I would like to note that any group-based splitter could well have very imbalanced test set sizes and users would get very different scores and best estimators if the default changed... But such cases should not, in theory, have used iid=True.

…

On 17 Jul 2017 8:19 pm, "Joel Nothman" ***@***.***> wrote: that's a fair point. On 17 Jul 2017 7:52 pm, "Olivier Grisel" ***@***.***> wrote: I am afraid that the FutureWarning is going to be very annoying. It cannot be silenced off by passing iid=False explicitly because this would trigger a DeprecationWarning and we don't want to encourage our users to explicit use the iid parameter in their code. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#9103 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz67yLb7JIsq07F51Z8ewVes9U_R1qks5sOy7ngaJpZM4N2JFL> .

amueller · 2017-07-18T14:55:57Z

So we don't really have a consensus, right?
Options are

1- warn to change in two versions (my PR: #9379). Noisy but can be silenced but takes 4 releases to go away.
2- change now and raise ChangedBehaviorWarning (or something) for two versions. Noisy and can't be silenced - except for setting a filter for ChangedBehaviorWarning -- which isn't that bad?
3- Change now and document in whatsnew.rst and don't warn.

When I created my PR I thought the change was much more significant. I agree that it will be very small in most cases. It's hard to warn selectively as even KFold and StratifiedKFold can create these warnings.

One option would be to only warn for splitters where the folds have very uneven sizes, so the grouped ones, the TimeSeriesSplit and any custom ones.
So I guess that's a fourth option - or rather fourth and fifth, because it could be combined with the "change now" (option 4) or the "warn about future change" (option 5).

ogrisel · 2017-07-18T17:15:29Z

@GaelVaroquaux what's your opinion?

jnothman · 2017-07-18T23:54:02Z

similarly to warning only if it's an imbalanced cv strategy, we could warn if the weighted and unweighted means differ greatly, but "greatly" is metric-dependent. I don't really see that we lose anything with the current (slow) approach. It maximises awareness of the change and minimises incompatibilities. On 19 Jul 2017 3:15 am, "Olivier Grisel" <notifications@github.com> wrote: @GaelVaroquaux <https://github.com/gaelvaroquaux> what's your opinion? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#9103 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz67YaLV4KcbuwIhGxbbfopdEllM0rks5sPOg0gaJpZM4N2JFL> .

amueller · 2017-07-31T14:33:27Z

It would be good to get input from @GaelVaroquaux before releasing

ldirer changed the title ~~[WIP] Add deprecation warning for iid in BaseSearchCV~~ [MRG] Add deprecation warning for iid in BaseSearchCV Jun 10, 2017

amueller reviewed Jun 10, 2017

View reviewed changes

ldirer changed the title ~~[MRG] Add deprecation warning for iid in BaseSearchCV~~ [WIP] Add deprecation warning for iid in BaseSearchCV Jun 10, 2017

ldirer added 2 commits June 17, 2017 16:59

Add deprecation warning for iid in BaseSearchCV

d0fdb4e

Revert changes on deprecated class and add deprecation to refactored …

be31367

…model_selection module

ldirer force-pushed the deprecate-iid-gridsearch branch from e513b8f to 451811b Compare June 17, 2017 15:01

Adding deprecation to changelog

7ad4b06

ldirer force-pushed the deprecate-iid-gridsearch branch from 451811b to 7ad4b06 Compare June 17, 2017 15:23

ldirer changed the title ~~[WIP] Add deprecation warning for iid in BaseSearchCV~~ [MRG] Add deprecation warning for iid in BaseSearchCV Jun 17, 2017

jnothman reviewed Jun 18, 2017

View reviewed changes

amueller added this to the 0.19 milestone Jul 15, 2017

amueller added the Blocker label Jul 15, 2017

amueller mentioned this pull request Jul 16, 2017

[MRG+1] GridSearchCV iid #9379

Merged

ogrisel modified the milestones: 0.20, 0.19 Jul 26, 2017

jnothman closed this in #9379 Dec 11, 2017

murphyk mentioned this pull request Feb 7, 2021

ML-dask binder example uses to-be-deprecated iid flag in GridSearchCV dask/dask-ml#791

Closed

techsword mentioned this pull request May 8, 2023

Removing iid gchrupala/ursa#4

Merged

jackred mentioned this pull request Mar 5, 2024

GridSearchCV do not weight the score by the size of the fold when providing custom split for CV #28575

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] Add deprecation warning for iid in BaseSearchCV #9103

[MRG] Add deprecation warning for iid in BaseSearchCV #9103

ldirer commented Jun 10, 2017 •

edited

Loading

vene commented Jun 10, 2017

amueller Jun 10, 2017

ldirer commented Jun 10, 2017

vene commented Jun 10, 2017

GaelVaroquaux commented Jun 10, 2017 via email

vene commented Jun 11, 2017

jnothman Jun 18, 2017

raghavrv commented Jul 13, 2017 •

edited

Loading

amueller commented Jul 15, 2017

amueller commented Jul 16, 2017

amueller commented Jul 16, 2017

amueller commented Jul 16, 2017

amueller commented Jul 16, 2017

agramfort commented Jul 16, 2017 via email

jnothman commented Jul 16, 2017 via email

agramfort commented Jul 17, 2017 via email

ogrisel commented Jul 17, 2017 •

edited

Loading

jnothman commented Jul 17, 2017 via email

ogrisel commented Jul 17, 2017 •

edited

Loading

jnothman commented Jul 17, 2017 via email •

edited by TomDLT

Loading

jnothman commented Jul 17, 2017 via email

amueller commented Jul 18, 2017 •

edited by ogrisel

Loading

ogrisel commented Jul 18, 2017

jnothman commented Jul 18, 2017 via email

amueller commented Jul 31, 2017

[MRG] Add deprecation warning for iid in BaseSearchCV #9103

[MRG] Add deprecation warning for iid in BaseSearchCV #9103

Conversation

ldirer commented Jun 10, 2017 • edited Loading

Reference Issue

What does this implement/fix? Explain your changes.

Any other comments?

vene commented Jun 10, 2017

amueller Jun 10, 2017

Choose a reason for hiding this comment

ldirer commented Jun 10, 2017

vene commented Jun 10, 2017

GaelVaroquaux commented Jun 10, 2017 via email

vene commented Jun 11, 2017

jnothman Jun 18, 2017

Choose a reason for hiding this comment

raghavrv commented Jul 13, 2017 • edited Loading

amueller commented Jul 15, 2017

amueller commented Jul 16, 2017

amueller commented Jul 16, 2017

amueller commented Jul 16, 2017

amueller commented Jul 16, 2017

agramfort commented Jul 16, 2017 via email

jnothman commented Jul 16, 2017 via email

agramfort commented Jul 17, 2017 via email

ogrisel commented Jul 17, 2017 • edited Loading

jnothman commented Jul 17, 2017 via email

ogrisel commented Jul 17, 2017 • edited Loading

jnothman commented Jul 17, 2017 via email • edited by TomDLT Loading

jnothman commented Jul 17, 2017 via email

amueller commented Jul 18, 2017 • edited by ogrisel Loading

ogrisel commented Jul 18, 2017

jnothman commented Jul 18, 2017 via email

amueller commented Jul 31, 2017

ldirer commented Jun 10, 2017 •

edited

Loading

raghavrv commented Jul 13, 2017 •

edited

Loading

ogrisel commented Jul 17, 2017 •

edited

Loading

ogrisel commented Jul 17, 2017 •

edited

Loading

jnothman commented Jul 17, 2017 via email •

edited by TomDLT

Loading

amueller commented Jul 18, 2017 •

edited by ogrisel

Loading