Skip to content
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

[MRG+1] Changes default for return_train_score to False #9677

Merged
merged 73 commits into from Oct 17, 2017

Conversation

@thechargedneutron
Copy link
Contributor

@thechargedneutron thechargedneutron commented Sep 2, 2017

Reference Issue

Fixes #9621

What does this implement/fix? Explain your changes.

Changes the default value of return_train_score to warn. Raises a warning if train score takes more time.

Any other comments?

The warning message needs improvement.

@thechargedneutron
Copy link
Contributor Author

@thechargedneutron thechargedneutron commented Sep 2, 2017

@jnothman Kindly suggest a suitable warning message.
Also please review. I guess this would need a lot of changes, mostly in documentations.

train_scores = _score(estimator, X_train, y_train, scorer,
is_multimetric)
score_train_time = time.time() - start_time - fit_time - score_time
if (score_train_time >= 0.1*fit_time and
time.time() - start_time) > 5:
Copy link
Member

@jnothman jnothman Sep 3, 2017

Parentheses in a strange place here...

And can't compare absolute time to 5 in a single fit. It will often only be a substantial amount of time over a large number of fits, as in the raising issue.

Copy link
Contributor Author

@thechargedneutron thechargedneutron Sep 3, 2017

How about adding a new variable alongside return_train_score in BaseSearchCV, containing the time of the first call to GridSearchCV. Also we need to pass that time variable to the _fit_and_score function.

@thechargedneutron
Copy link
Contributor Author

@thechargedneutron thechargedneutron commented Sep 3, 2017

@jnothman Added a parameter in the function which takes care of the actual start time of GridSearchCV. Please see if this is valid or not.

Copy link
Member

@jnothman jnothman left a comment

It's not a bad approach, but I suspect it won't receive the consensus to get merge, just because it makes the _fit_and_score interface more messy...

@amueller, what do you think of this approach?

train_scores = _score(estimator, X_train, y_train, scorer,
is_multimetric)
score_train_time = time.time() - start_time - fit_time - score_time
if score_train_time >= 0.1*fit_time and time.time() - grid_search_start_time> 5:
Copy link
Member

@jnothman jnothman Sep 3, 2017

This still may mean that training score calculation time took only .5 of a second, which I don't think is quite enough. Maybe better would be to (naively) estimate overall training score time: (time.time() - grid_search_start_time) * score_train_time / (fit_time + score_time + score_train_time) > 5..?

@thechargedneutron
Copy link
Contributor Author

@thechargedneutron thechargedneutron commented Sep 3, 2017

@jnothman I also agree upon the fact that it makes the _fit_and_score method more messy. But I could not find and alternative to keep track of total time that GridSearchCV would take. You or @amueller may suggest a way which serves the purpose without changing _fit_and_score interface.

@jnothman
Copy link
Member

@jnothman jnothman commented Sep 3, 2017

@thechargedneutron
Copy link
Contributor Author

@thechargedneutron thechargedneutron commented Sep 4, 2017

@amueller @lesteve Any suggestions on whether I should go on to change _fit_and_score method interface or would it be better to raise the warning only after the call to the function _fit_and_score ends?

@thechargedneutron
Copy link
Contributor Author

@thechargedneutron thechargedneutron commented Sep 6, 2017

@jnothman Should I continue with changing the function interface or raise warning only after completing _fit_and_score call ?

@jnothman
Copy link
Member

@jnothman jnothman commented Sep 7, 2017

@lesteve
Copy link
Member

@lesteve lesteve commented Sep 7, 2017

What about the seemingly simpler solution of changing the default to return_train_score=False, potentially by adding a FutureWarning that the default is going to be return_train_score=False in 0.22?

@thechargedneutron
Copy link
Contributor Author

@thechargedneutron thechargedneutron commented Sep 7, 2017

@lesteve Yes, this is also a simple solution, will be done once others agree upon this.

@jnothman
Copy link
Member

@jnothman jnothman commented Sep 7, 2017

@thechargedneutron
Copy link
Contributor Author

@thechargedneutron thechargedneutron commented Sep 7, 2017

@jnothman Kindly review and suggest a suitable FutureWarning message.

@thechargedneutron thechargedneutron changed the title [WIP] Adds warning in GridSearchCV if calculating train score is unduly expensive. [MRG] Adds warning in GridSearchCV if calculating train score is unduly expensive. Sep 7, 2017
@thechargedneutron
Copy link
Contributor Author

@thechargedneutron thechargedneutron commented Sep 7, 2017

Suggestions for a "nicer" warning message. Otherwise, I think this will work. :)

@jnothman jnothman changed the title [MRG] Adds warning in GridSearchCV if calculating train score is unduly expensive. [MRG] Changes default for return_train_score to False Sep 8, 2017
@thechargedneutron
Copy link
Contributor Author

@thechargedneutron thechargedneutron commented Oct 12, 2017

Even after doing the required changes (change in test still not made), the following lines of code does not produce a deprecation warning, is it how DeprecationDict is supposed to behave or I am missing out something.

from sklearn import svm, datasets
from sklearn.model_selection import GridSearchCV
iris = datasets.load_iris()
parameters = {'kernel':('linear','rbf'), 'C':[1,10]}
svc = svm.SVC()
clf = GridSearchCV(svc, parameters)
clf.fit(iris.data, iris.target)
sorted(clf.cv_results_.keys())

@lesteve
Copy link
Member

@lesteve lesteve commented Oct 12, 2017

Warning should be produced when accessing the cv_results_ key for training scores:

from sklearn import svm, datasets
from sklearn.model_selection import GridSearchCV
iris = datasets.load_iris()
parameters = {'kernel':('linear','rbf'), 'C':[1,10]}
svc = svm.SVC()
clf = GridSearchCV(svc, parameters)
clf.fit(iris.data, iris.target)
clf.cv_results_['split0_train_score']  # this is the line that should produce a warning

Note that you should not get any warning if you use return_train_score=True.

from sklearn import svm, datasets
from sklearn.model_selection import GridSearchCV
iris = datasets.load_iris()
parameters = {'kernel':('linear','rbf'), 'C':[1,10]}
svc = svm.SVC()
clf = GridSearchCV(svc, parameters, return_train_score=True)
clf.fit(iris.data, iris.target)
clf.cv_results_['split0_train_score']  # no warning because return_train_score is set to True

Basically what we want users who do not set return_train_score and access cv results train score to get a warning and tell them they should set return_train_score to True because training scores will not be present by default in 0.20.

@thechargedneutron
Copy link
Contributor Author

@thechargedneutron thechargedneutron commented Oct 12, 2017

I am not sure how DeprecationDict is working. Need help on how to implement.

@lesteve
Copy link
Member

@lesteve lesteve commented Oct 13, 2017

You need to help us help you!

Can you be specific about your problem is? what have you tried? If there is a failure that you don't understand, can you copy and paste it here?

@jnothman
Copy link
Member

@jnothman jnothman commented Oct 16, 2017

See my PR at thechargedneutron#2

@lesteve
Copy link
Member

@lesteve lesteve commented Oct 16, 2017

I restarted the failing build in Travis hoping the timeout was just a glitch.

@jnothman
Copy link
Member

@jnothman jnothman commented Oct 16, 2017

Do you think this approach is decent, @lesteve? Too messy? It implies that a user no longer gets a warning informing them why the fit is so slow which was the original point...!

@lesteve
Copy link
Member

@lesteve lesteve commented Oct 16, 2017

I have not looked at the diff of this PR (will do shortly). To be perfectly honest, I think your DeprecationDict suggestions is really neat and this is the best we can do:

  • we tried to have a warning only if the scoring for train was slow and we failed because it was a bit messy to implement. Personally I think return_train_score=False is a reasonable default (compute more things only if asked by the user).
  • I think the DeprecationDict approach is the best way to transition from return_train_score=True to return_train_score=False. People will get the warning if they do not set return_train_score and access and then they can decide what they really want.
  • In 0.21 return_train_score=False will be the default so the code will be fast by default.

"validation significantly. This is the reason "
"return_train_score will change its default value "
"from True (current behaviour) to False in 0.21. "
"Please set return_train_score explicitly to get "
Copy link
Member

@lesteve lesteve Oct 17, 2017

I think this message should be changed slightly to say something along the lines of "Looks like you are using training scores, you want to set return_train_score=True because return_train_score default will change in 0.21"

@lesteve
Copy link
Member

@lesteve lesteve commented Oct 17, 2017

@jnothman I pushed two main changes, it would be nice to have your opinion on these:

  • I simplified the warning message, since it only happens when accessing a training score
  • DeprecationDict is only used when return_train_score='warn'. This is me being overly cautious/pessimistic mainly and preferring to use a plain dict when return_train_score is set explicitly. I can revert this if you feel this is too much.

I am going to reset the LGTM count and add a +1 from me.

@lesteve lesteve changed the title [MRG+2?] Changes default for return_train_score to False [MRG+1] Changes default for return_train_score to False Oct 17, 2017
'which will not be available by default '
'any more in 0.21. If you need training scores, '
'please set return_train_score=True').format(key)
train_score = assert_warns_message(FutureWarning, msg,
Copy link
Member

@amueller amueller Oct 17, 2017

shouldn't we assert that there is no warning for the other vals and that there is no warning for the other keys for 'warn'?

Otherwise LGTM.

@jnothman jnothman merged commit 766ba93 into scikit-learn:master Oct 17, 2017
1 of 4 checks passed
1 of 4 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
lgtm analysis: Python Running analyses for revisions
Details
ci/circleci Your tests passed on CircleCI!
Details
jnothman added a commit that referenced this issue Oct 17, 2017
@jnothman
Copy link
Member

@jnothman jnothman commented Oct 17, 2017

Thanks all

jnothman added a commit to jnothman/scikit-learn that referenced this issue Oct 17, 2017
@thechargedneutron thechargedneutron deleted the searchcv branch Oct 18, 2017
maskani-moh added a commit to maskani-moh/scikit-learn that referenced this issue Nov 15, 2017
jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this issue Dec 18, 2017
jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this issue Dec 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants