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] Change default error_score to raise_deprecating #10677

Merged
merged 7 commits into from Mar 2, 2018

Conversation

Projects
None yet
3 participants
@Zhdanovich
Contributor

Zhdanovich commented Feb 22, 2018

Reference Issues/PRs

Fixes #10309.

What does this implement/fix? Explain your changes.

Changes default parameter of error_score to raise-deprecating

Any other comments?

Should be method _fit_and_score(cross_validation.py) be updated in the same way?

@Zhdanovich Zhdanovich changed the title from Change default error_score to raise_deprecating to [MRG] Change default error_score to raise_deprecating Feb 22, 2018

@jnothman

We also need to

  • Test that the FutureWarning is raised, perhaps only in the case that the fit errors
  • Test that if a score is NaN *SearchCV does not select this as the best score because np.argmax([1, np.nan]) == 1.

Sorry to make this more complicated, though you've done a good job so far. Thanks!

@@ -311,11 +311,14 @@ def fit_grid_point(X, y, estimator, parameters, train, test, scorer,
**fit_params : kwargs
Additional parameter passed to the fit function of the estimator.
error_score : 'raise' (default) or numeric
error_score : 'raise-deprecating' (default), 'raise' or numeric

This comment has been minimized.

@jnothman

jnothman Feb 22, 2018

Member

I think of raise-deprecating as an implementation detail that we shouldn't document.

Rather, we need to say that From version 0.22 the default will be np.nan

@@ -464,6 +466,13 @@ def _fit_and_score(estimator, X, y, scorer, train, test, verbose,
score_time = 0.0
if error_score == 'raise':
raise
elif error_score == 'raise-deprecating':
warnings.warn("Estimator fit failed. In the future,"
"raise-deprecating error will not be"

This comment has been minimized.

@jnothman

jnothman Feb 22, 2018

Member

No need to reference raise-deprecating, just explain how the default error_score behaviour will change

"raise-deprecating error will not be"
"used as a default value. There will be"
"possibility to choose between error_score='raise',"
"numeric or the new default error_score=np.nan")

This comment has been minimized.

@jnothman

jnothman Feb 22, 2018

Member

Make this a FutureWarning

@@ -1450,3 +1450,12 @@ def test_fit_and_score():
# check if the same warning is triggered
assert_warns_message(FitFailedWarning, warning_message, _fit_and_score,
*fit_and_score_args, **fit_and_score_kwargs)
# check if exception was raised, with default error_score argument
assert_raise_message(ValueError, "", _fit_and_score,

This comment has been minimized.

@jnothman

jnothman Feb 22, 2018

Member

Why do we not test for the message text?

This comment has been minimized.

@Zhdanovich

Zhdanovich Feb 22, 2018

Contributor

Can you please elaborate on this? Maybe I'm wrong, but when raise method is invoked there is no message.

This comment has been minimized.

@jnothman

jnothman Feb 22, 2018

Member

It should be the original error from fit, in this case "Failing classifier failed as required"

@Zhdanovich Zhdanovich changed the title from [MRG] Change default error_score to raise_deprecating to [WIP] Change default error_score to raise_deprecating Feb 22, 2018

@Zhdanovich

This comment has been minimized.

Contributor

Zhdanovich commented Feb 23, 2018

@jnothman "Test that if a score is NaN *SearchCV does not select this as the best score because np.argmax([1, np.nan]) == 1." - should this be done now? Feels like it's necessary to do when an actual change(error_score=np.nan) is in place.
It's already possible to pass np.nan as error_score, this will break current behavior, not sure if anyone uses it in this way.

@Zhdanovich Zhdanovich changed the title from [WIP] Change default error_score to raise_deprecating to [MRG] Change default error_score to raise_deprecating Feb 23, 2018

@jnothman

This comment has been minimized.

Member

jnothman commented Feb 24, 2018

@Zhdanovich

This comment has been minimized.

Contributor

Zhdanovich commented Feb 24, 2018

@jnothman is not ok to merge this now. And I start on another part on Monday?

@jnothman

This comment has been minimized.

Member

jnothman commented Feb 25, 2018

We'll only merge with two core devs approving. But you can start on the other pr simultaneously

@jnothman

otherwise lgtm

@@ -464,6 +464,12 @@ def _fit_and_score(estimator, X, y, scorer, train, test, verbose,
score_time = 0.0
if error_score == 'raise':
raise
elif error_score == 'raise-deprecating':
warnings.warn("Estimator fit failed. From version 0.22"

This comment has been minimized.

@jnothman

jnothman Feb 25, 2018

Member

missing a space between "0.22" and "the"

elif error_score == 'raise-deprecating':
warnings.warn("Estimator fit failed. From version 0.22"
"the default valuer will be error_score=np.nan",

This comment has been minimized.

@jnothman

jnothman Feb 25, 2018

Member

valuer -> value

However, I think this is better expressed with something like "From version 0.22, errors during fit will result in a cross validation score of NaN by default. Use error_score='raise' if you want an exception raised."

" numeric value. (Hint: if using 'raise', please"
" make sure that it has been spelled correctly.)")
raise ValueError("error_score must be the string 'raise',"
"'raise-deprecating' or a numeric value. (Hint:"

This comment has been minimized.

@jnothman

jnothman Feb 25, 2018

Member

We don't want users to ever manually specify 'raise-deprecating', so please just don't mention it.

@Zhdanovich

This comment has been minimized.

Contributor

Zhdanovich commented Feb 25, 2018

@jnothman updated

@jnothman

This comment has been minimized.

Member

jnothman commented Feb 25, 2018

Please append commits rather than amending previous commits. It makes it much easier to track what's changed from review to review.

elif error_score == 'raise-deprecating':
warnings.warn("Estimator fit failed. From version 0.22 "
"the default value will be error_score=np.nan",

This comment has been minimized.

@jnothman

jnothman Feb 25, 2018

Member

I suggested a more verbose error message here because I don't believe that every user who encounters this message will be familiar with error_score or appreciate what "the default value will be" means to them

@Zhdanovich

This comment has been minimized.

Contributor

Zhdanovich commented Feb 25, 2018

@jnothman just to be sure about another part. If a user doesn't specify error_score(error_score=np.nan) and estimator fails to fit, then we filter such cases when checking the best parameters?

@jnothman

This comment has been minimized.

Member

jnothman commented Feb 25, 2018

If a user doesn't specify error_score(error_score=np.nan) and estimator fails to fit, then we filter such cases when checking the best parameters?

If it's raising an error then there's no such thing as a best fit. If the user specifies a non-NaN number, then we treat that as currently handled: sort the mean score. If it's np.NaN then the mean score becomes NaN, and the only problem is that NaN sorts as a higher value than any other:

>>> np.argsort(np.asarray([np.nan, 1, 2, 3]))
array([1, 2, 3, 0])

However, it looks like scipy.stats.rankdata, which is what we use in BaseSearchCV to determine the best system, behaves differently, and always assigns np.NaN the lowest rank:

>>> from scipy.stats import rankdata
>>> rankdata(np.asarray([np.nan, 1, 2, 3]))
array([4., 1., 2., 3.])
>>> rankdata(-np.asarray([np.nan, 1, 2, 3]))
array([4., 3., 2., 1.])

So what we have already is probably working correctly, but it needs a test (which I don't think we already have).

@jnothman

This comment has been minimized.

Member

jnothman commented Feb 25, 2018

(We don't test that despite testing other aspects of error_score=NaN in test_grid_search_failing_classifier. Because we set all fits to fail, we can't see what happens when comparing a NaN score to a non-NaN score.)

1 similar comment
@jnothman

This comment has been minimized.

Member

jnothman commented Feb 25, 2018

(We don't test that despite testing other aspects of error_score=NaN in test_grid_search_failing_classifier. Because we set all fits to fail, we can't see what happens when comparing a NaN score to a non-NaN score.)

@jnothman

grammar nitpicks

@@ -464,6 +464,15 @@ def _fit_and_score(estimator, X, y, scorer, train, test, verbose,
score_time = 0.0
if error_score == 'raise':
raise
elif error_score == 'raise-deprecating':
warnings.warn("Estimator failed to fit train-test for this "

This comment has been minimized.

@jnothman

jnothman Feb 25, 2018

Member

this -> these. Drop "train-test".

elif error_score == 'raise-deprecating':
warnings.warn("Estimator failed to fit train-test for this "
"parameters, the error will be raised. If you want "

This comment has been minimized.

@jnothman

jnothman Feb 25, 2018

Member

, -> ;

"then specify error_score parameter. From version "
"0.22 the default value will be error_score=np.nan",
warnings.warn("Estimator failed to fit for these parameters; the "
"error will be raised. If you want value to be "

This comment has been minimized.

@jnothman

jnothman Feb 25, 2018

Member

really should be "want a value"

"0.22 the default value will be error_score=np.nan",
warnings.warn("Estimator failed to fit for these parameters; the "
"error will be raised. If you want value to be "
"assigned to the score in such case, then specify "

This comment has been minimized.

@jnothman

jnothman Feb 25, 2018

Member

really should be "such a case" or "such cases"

@jnothman

Thanks, LGTM

@Zhdanovich Zhdanovich changed the title from [MRG] Change default error_score to raise_deprecating to [MRG+1] Change default error_score to raise_deprecating Feb 26, 2018

@Zhdanovich

This comment has been minimized.

Contributor

Zhdanovich commented Feb 26, 2018

@jnothman thanks a lot for the help! How it usually works, how I can get a second reviewer?

@jnothman

This comment has been minimized.

Member

jnothman commented Feb 26, 2018

@rasbt rasbt referenced this pull request Feb 27, 2018

Open

FEA Sequential feature selection #8684

0 of 2 tasks complete
@Zhdanovich

This comment has been minimized.

Contributor

Zhdanovich commented Feb 27, 2018

@rth please take a look

@Zhdanovich

This comment has been minimized.

Contributor

Zhdanovich commented Feb 27, 2018

@qinhanmin2014 please take a look

@rth

This comment has been minimized.

Member

rth commented Feb 27, 2018

Thanks for this PR @Zhdanovich

I think of raise-deprecating as an implementation detail that we shouldn't document.

@jnothman I see your point, but it's still kind of unexpected to have a default value not documented for a couple of versions. Anyone looking at the docs (latest build here) or the code will wonder what the default value actually does.

Instead of

From version 0.22 the default will be np.nan

how about

Default is 'raise' but from version 0.22 it will change to np.nan.

particularly since most users will never see the warning as it requires for grid search to fail, which is not that frequent. WDYT?

assert_raise_message(ValueError, "Failing classifier failed as required",
_fit_and_score, *fit_and_score_args)
# check if warning was raised, with default error_score argument

This comment has been minimized.

@rth

rth Feb 27, 2018

Member

Nitpick, but I think "is raised" in the present is more common (in all 3 comments): after all, it's "assert_raise_message" not "asssert_raised_message".

"default value will be error_score=np.nan")
try:
assert_warns_message(FutureWarning, warning_message, _fit_and_score,
*fit_and_score_args)

This comment has been minimized.

@rth

rth Feb 27, 2018

Member

Could we use,

with pytest.raises(ValueError):
    assert_warns_message(..)

here? As this try: except: pass leaves the reader puzzled as to whether it will raise an exception or not.

@Zhdanovich

This comment has been minimized.

Contributor

Zhdanovich commented Feb 28, 2018

@rth thanks for the review.

how about

Default is 'raise' but from version 0.22 it will change to np.nan.

But it's also not true since the default value is raise-deprecating.
ping @jnothman

@jnothman

This comment has been minimized.

Member

jnothman commented Feb 28, 2018

@Zhdanovich

This comment has been minimized.

Contributor

Zhdanovich commented Feb 28, 2018

@rth updated. Please take a look

Value to assign to the score if an error occurs in estimator fitting.
If set to 'raise', the error is raised. If a numeric value is given,
FitFailedWarning is raised. This parameter does not affect the refit
step, which will always raise the error.
step, which will always raise the error. From version 0.22 the default
will be np.nan

This comment has been minimized.

@rth

rth Feb 28, 2018

Member

"." is missing at the end. Also could you please change the last sentence to match the message in other docstrings?

"error will be raised. If you want a value to be "
"assigned to the score in such cases, then specify "
"error_score parameter. Default is 'raise' but from "
"version 0.22 it will change to np.nan.",

This comment has been minimized.

@rth

rth Feb 28, 2018

Member

I think this warning should be more clear a) about why a user is getting it b) how to make it go away.
Something along the lines of what @jnothman said in #10677 (comment) might be better I think,

"From version 0.22, errors during fit will result in a cross validation score of NaN by default. Use error_score='raise' if you want an exception raised."

Once sentence for a) and one for b) . and maybe add "[...] or error_score=np.nan to adopt the behavior from version 0.22."

@Zhdanovich

This comment has been minimized.

Contributor

Zhdanovich commented Mar 1, 2018

@rth please take a look

@rth

This comment has been minimized.

Member

rth commented Mar 1, 2018

Just one space too many after "or" in raised or error_score=np.nan in 2 warning messages.

Otherwise LGTM. Thanks @Zhdanovich

@Zhdanovich

This comment has been minimized.

Contributor

Zhdanovich commented Mar 2, 2018

@rth Thanks. Updated!

@jnothman jnothman merged commit 76b5b2a into scikit-learn:master Mar 2, 2018

8 checks passed

ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: python2 Your tests passed on CircleCI!
Details
ci/circleci: python3 Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 95.01%)
Details
codecov/project 95.01% (+<.01%) compared to 02ddc70
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: Python No alert changes
Details
@jnothman

This comment has been minimized.

Member

jnothman commented Mar 2, 2018

Thanks!

@jnothman

Actually, now that I've merged, I've realised two small things are wrong:

  • missing an entry in doc/whats_new/v0.20.rst
  • we've changed _fit_and_score's default, and we should not have, because cross_validate and cross_val_score do not (currently) provide an error_score option. Maybe that's something else that should change. But at a minimum, we need to make sure that cross_validate doesn't raise an irrelevant FutureWarning.
@Zhdanovich

This comment has been minimized.

Contributor

Zhdanovich commented Mar 2, 2018

@jnothman I can have a look

@Zhdanovich

This comment has been minimized.

Contributor

Zhdanovich commented Mar 2, 2018

Now I see that I asked this question in "Any other comments?" section

@Zhdanovich

This comment has been minimized.

Contributor

Zhdanovich commented Mar 2, 2018

@jnothman

we've changed _fit_and_score's default, and we should not have, because cross_validate and cross_val_score do not (currently) provide an error_score option. Maybe that's something else that should change. But at a minimum, we need to make sure that cross_validate doesn't raise an irrelevant FutureWarning.

Should I for now pass error_score='raise' to _fit_and_score methods in cross_validate and cross_val_score?

ccatalfo added a commit to ccatalfo/scikit-learn that referenced this pull request Mar 5, 2018

@Zhdanovich

This comment has been minimized.

Contributor

Zhdanovich commented Mar 14, 2018

@jnothman Hello, can you check my previous comment, please?

@jnothman

This comment has been minimized.

Member

jnothman commented Mar 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment