Warn in the main process when a fit fails during a cross-validation #20619

lesteve · 2021-07-27T15:33:45Z

Reference Issues/PRs

Fix #20475

What does this implement/fix? Explain your changes.

Removes a per-split warning and do a summary warning in the main process.

Any other comments?

Still polishing it for now, some test failures expected.

Some remaining things to discuss/do:

removing the per-split warning may affect BaseSearchCV derived classes outside scikit-learn. There will be no warnings when a fit fails. How acceptable is it?
agree on the warning message. This is how it looks right now. I could potentially shorten it when some of the errors are exactly the same:

/home/local/lesteve/dev/scikit-learn/sklearn/model_selection/_validation.py:366: FitFailedWarning: 
5 fits failed on the training sets over a total of 5 fits. The score on these train-test partitions for these parameters will be set to nan.
In case these failures are not expected, you can try to debug these failures by setting error_score='raise'.

Here are more details about the failures:
----------
Traceback (most recent call last):
  File "/home/local/lesteve/dev/scikit-learn/sklearn/model_selection/_validation.py", line 675, in _fit_and_score
    estimator.fit(X_train, y_train, **fit_params)
  File "/home/local/lesteve/dev/scikit-learn/sklearn/linear_model/_logistic.py", line 1458, in fit
    raise ValueError("Penalty term must be positive; got (C=%r)" % self.C)
ValueError: Penalty term must be positive; got (C='wrong-value')
----------
Traceback (most recent call last):
  File "/home/local/lesteve/dev/scikit-learn/sklearn/model_selection/_validation.py", line 675, in _fit_and_score
    estimator.fit(X_train, y_train, **fit_params)
  File "/home/local/lesteve/dev/scikit-learn/sklearn/linear_model/_logistic.py", line 1458, in fit
    raise ValueError("Penalty term must be positive; got (C=%r)" % self.C)
ValueError: Penalty term must be positive; got (C='wrong-value')
----------
Traceback (most recent call last):
  File "/home/local/lesteve/dev/scikit-learn/sklearn/model_selection/_validation.py", line 675, in _fit_and_score
    estimator.fit(X_train, y_train, **fit_params)
  File "/home/local/lesteve/dev/scikit-learn/sklearn/linear_model/_logistic.py", line 1458, in fit
    raise ValueError("Penalty term must be positive; got (C=%r)" % self.C)
ValueError: Penalty term must be positive; got (C='wrong-value')
----------
Traceback (most recent call last):
  File "/home/local/lesteve/dev/scikit-learn/sklearn/model_selection/_validation.py", line 675, in _fit_and_score
    estimator.fit(X_train, y_train, **fit_params)
  File "/home/local/lesteve/dev/scikit-learn/sklearn/linear_model/_logistic.py", line 1458, in fit
    raise ValueError("Penalty term must be positive; got (C=%r)" % self.C)
ValueError: Penalty term must be positive; got (C='wrong-value')
----------
Traceback (most recent call last):
  File "/home/local/lesteve/dev/scikit-learn/sklearn/model_selection/_validation.py", line 675, in _fit_and_score
    estimator.fit(X_train, y_train, **fit_params)
  File "/home/local/lesteve/dev/scikit-learn/sklearn/linear_model/_logistic.py", line 1458, in fit
    raise ValueError("Penalty term must be positive; got (C=%r)" % self.C)
ValueError: Penalty term must be positive; got (C='wrong-value')

  warnings.warn(some_fits_failed_message, FitFailedWarning)

add changelog
test_callable_multimetric_clf_all_fails failure: quirk that for multimetric you get an error if all the test fails (but not for single metric). Is there a good reason for this behaviour? I changed my PR to make this test pass, basically the warnings on fit failures happens before the insertion of error scores for the multimetric case.

Putting _warn_about_fit_failures before _insert_error_scores is enough.

lesteve · 2021-07-28T15:30:38Z

The tests pass, the CI red only because I have not added a changelog yet. Comments about the points noted in my top post or anything else in this PR, let me know!

lesteve · 2021-07-28T15:32:22Z

sklearn/model_selection/tests/test_validation.py

@@ -2082,37 +2082,6 @@ def test_fit_and_score_failing():
    y = np.ones(9)
    fit_and_score_args = [failing_clf, X, None, dict(), None, None, 0, None, None]
    # passing error score to trigger the warning message
-    fit_and_score_kwargs = {"error_score": 0}


The check on the warnings were moved below to test_cross_validate_failing_fits_warnings since _fit_and_score does not warn anymore

thomasjpfan

removing the per-split warning may affect BaseSearchCV derived classes outside scikit-learn.

The BaseSearchCV "public API" consist calling evaluate_candidates in _run_search. The warning will still appear when calling evaluate_candidates, just not as often. I think it is okay to change.

This is how it looks right now. I could potentially shorten it when some of the errors are exactly the same:

If error message is exactly the same, only show one of them?

quirk that for multimetric you get an error if all the test fails (but not for single metric)

With callable multimetric, at least one _fit_and_score has to succeed so that *SearchCV can create error_score dictionaries for the failed cases. In the single metric case, *SearchCV can still proceed because there are no keys to deal with.

sklearn/model_selection/_validation.py

glemaitre · 2021-07-29T12:44:28Z

The BaseSearchCV "public API" consist calling evaluate_candidates in _run_search. The warning will still appear when calling evaluate_candidates, just not as often. I think it is okay to change.

To confirm this behaviour, I assume that we can try using HalvingSearchCV where the warning can be raised several times.

glemaitre · 2021-07-29T12:46:11Z

With callable multimetric, at least one _fit_and_score has to succeed so that *SearchCV can create error_score dictionaries for the failed cases. In the single metric case, *SearchCV can still proceed because it are no keys to deal with.

Interesting. On the user side, you don't think that we should anyway raise an error when everything is np.nan? I don't see an application where you can do anything with this result?

Indeed, I would not be surprised to raise an error in both multimeric and single metric. I would be a bit more lenient if some of the metrics fail in the multimetric case.

thomasjpfan · 2021-07-29T18:35:34Z

On the user side, you don't think that we should anyway raise an error when everything is np.nan?

It does make sense to raise an error if everything failed. We can add this improvement in another PR.

lesteve · 2021-07-30T12:53:48Z

I summarised errors in the warnings showing each error only once. This is how the warning looks with 10 fit failures with 2 different failures:

❯ python test.py                                                         
/home/lesteve/dev/scikit-learn/sklearn/model_selection/_validation.py:372: FitFailedWarning: 
10 fits failed on the training sets over a total of 25 fits.
The score on these train-test partitions for these parameters will be set to nan.
If these failures are not expected, you can try to debug them by setting error_score='raise'.

Below are more details about the failures:
--------------------------------------------------------------------------------
5 fits failed with the following error:
Traceback (most recent call last):
  File "/home/lesteve/dev/scikit-learn/sklearn/model_selection/_validation.py", line 681, in _fit_and_score
    estimator.fit(X_train, y_train, **fit_params)
  File "/home/lesteve/dev/scikit-learn/test.py", line 16, in fit
    raise ValueError(f"Failing classifier failed for parameter {self.parameter}")
ValueError: Failing classifier failed for parameter 2

--------------------------------------------------------------------------------
5 fits failed with the following error:
Traceback (most recent call last):
  File "/home/lesteve/dev/scikit-learn/sklearn/model_selection/_validation.py", line 681, in _fit_and_score
    estimator.fit(X_train, y_train, **fit_params)
  File "/home/lesteve/dev/scikit-learn/test.py", line 16, in fit
    raise ValueError(f"Failing classifier failed for parameter {self.parameter}")
ValueError: Failing classifier failed for parameter 3

test.py script used

import numpy as np

from sklearn.base import BaseEstimator


class FailingClassifier(BaseEstimator):
    """Classifier that raises a ValueError on fit()"""

    FAILING_PARAMETERS = [2, 3]

    def __init__(self, parameter=None):
        self.parameter = parameter

    def fit(self, X, y=None):
        if self.parameter in FailingClassifier.FAILING_PARAMETERS:
            raise ValueError(f"Failing classifier failed for parameter {self.parameter}")

    def predict(self, X):
        return np.zeros(X.shape[0])

    def score(self, X=None, Y=None):
        return 0.0


from sklearn.datasets import make_classification
X, y = make_classification()

from sklearn.model_selection import GridSearchCV
gs = GridSearchCV(FailingClassifier(), {'parameter': [1, 2, 3, 4, 5]})
gs.fit(X, y)

ogrisel · 2021-07-30T13:14:12Z

That looks great!

sklearn/model_selection/_search.py

sklearn/model_selection/_validation.py

sklearn/model_selection/_search.py

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

jnothman · 2021-08-05T11:40:14Z

sklearn/model_selection/_validation.py

+        )
+
+        some_fits_failed_message = (
+            f"\n{num_failed_fits} fits failed on the training sets over a total of"


I find this language a bit strange... Should "over" be "out of"? Is "on the training sets" helpful?

Thanks for the comment, I updated the wording:

now: "5 fits failed out of a total of 15"

before it was: "5 fits failed on the training sets over a total of 15 fits"

Let me know if the language is still a bit strange!

ogrisel

LGTM. I think this is a really nice usability improvement.

+1 for a follow-up PR to always raise an error if 100% of the fits fail (both in simple cross validation and in hyperparam search).

doc/whats_new/v1.0.rst

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

thomasjpfan

Small nit, otherwise LGTM!

sklearn/model_selection/_validation.py

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

sklearn/model_selection/_validation.py

ogrisel · 2021-08-10T14:31:46Z

Merged! Thank you very much for the improvement @lesteve!

…cikit-learn#20619) Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

lesteve added 5 commits July 9, 2021 17:52

wip

e83e919

add script

1cdc553

wi

fb4e413

half working

911e5d5

clean

e327ff4

github-actions bot added the module:model_selection label Jul 27, 2021

lesteve marked this pull request as draft July 27, 2021 15:34

lesteve changed the title ~~wip~~ Warn in the main process when a fit fails during a cross-validation Jul 27, 2021

lesteve added 4 commits July 28, 2021 11:21

wip

161642c

Fix all tests but test_callable_multimetric_clf_all_fails

999832a

black

83c155b

Fix test_callable_multimetric_clf_all_fails.

932b771

Putting _warn_about_fit_failures before _insert_error_scores is enough.

lesteve marked this pull request as ready for review July 28, 2021 14:53

lesteve commented Jul 28, 2021

View reviewed changes

thomasjpfan reviewed Jul 28, 2021

View reviewed changes

sklearn/model_selection/_validation.py Show resolved Hide resolved

sklearn/model_selection/_validation.py Outdated Show resolved Hide resolved

lesteve added 3 commits July 30, 2021 14:27

Summarise similar failures in warning and update tests

84bdd5c

changelog + black

46e1f03

typo

9bee201

lesteve commented Jul 30, 2021

View reviewed changes

sklearn/model_selection/_search.py Outdated Show resolved Hide resolved

thomasjpfan reviewed Jul 31, 2021

View reviewed changes

sklearn/model_selection/_validation.py Outdated Show resolved Hide resolved

sklearn/model_selection/_validation.py Show resolved Hide resolved

sklearn/model_selection/_search.py Outdated Show resolved Hide resolved

sklearn/model_selection/_search.py Outdated Show resolved Hide resolved

lesteve and others added 3 commits August 3, 2021 10:36

Update sklearn/model_selection/_validation.py

180364e

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

Use out rather than all_out

9f0594e

Remove new line

9e80091

lesteve mentioned this pull request Aug 5, 2021

Halving*SearchCV selects estimator with nan score as best estimator #20678

Closed

jnothman reviewed Aug 5, 2021

View reviewed changes

Improve wording

ac3c765

ogrisel approved these changes Aug 6, 2021

View reviewed changes

glemaitre approved these changes Aug 6, 2021

View reviewed changes

doc/whats_new/v1.0.rst Show resolved Hide resolved

Update doc/whats_new/v1.0.rst

b1ab131

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

thomasjpfan approved these changes Aug 8, 2021

View reviewed changes

sklearn/model_selection/_validation.py Outdated Show resolved Hide resolved

Update sklearn/model_selection/_validation.py

69c2838

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

ogrisel reviewed Aug 10, 2021

View reviewed changes

sklearn/model_selection/_validation.py Outdated Show resolved Hide resolved

Update sklearn/model_selection/_validation.py

1cb1bc9

ogrisel merged commit 7317416 into scikit-learn:main Aug 10, 2021

lesteve deleted the cross-val-nan-score branch September 13, 2021 13:21

lesteve mentioned this pull request Sep 13, 2021

Raise an error when all fits fail in cross-validation or grid-search #21026

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn in the main process when a fit fails during a cross-validation #20619

Warn in the main process when a fit fails during a cross-validation #20619

lesteve commented Jul 27, 2021 •

edited

Loading

lesteve commented Jul 28, 2021

lesteve Jul 28, 2021

thomasjpfan left a comment •

edited

Loading

glemaitre commented Jul 29, 2021

glemaitre commented Jul 29, 2021 •

edited

Loading

thomasjpfan commented Jul 29, 2021

lesteve commented Jul 30, 2021

ogrisel commented Jul 30, 2021

jnothman Aug 5, 2021

lesteve Aug 5, 2021

ogrisel left a comment

thomasjpfan left a comment

ogrisel commented Aug 10, 2021

Warn in the main process when a fit fails during a cross-validation #20619

Warn in the main process when a fit fails during a cross-validation #20619

Conversation

lesteve commented Jul 27, 2021 • edited Loading

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

lesteve commented Jul 28, 2021

lesteve Jul 28, 2021

Choose a reason for hiding this comment

thomasjpfan left a comment • edited Loading

Choose a reason for hiding this comment

glemaitre commented Jul 29, 2021

glemaitre commented Jul 29, 2021 • edited Loading

thomasjpfan commented Jul 29, 2021

lesteve commented Jul 30, 2021

ogrisel commented Jul 30, 2021

jnothman Aug 5, 2021

Choose a reason for hiding this comment

lesteve Aug 5, 2021

Choose a reason for hiding this comment

ogrisel left a comment

Choose a reason for hiding this comment

thomasjpfan left a comment

Choose a reason for hiding this comment

ogrisel commented Aug 10, 2021

lesteve commented Jul 27, 2021 •

edited

Loading

thomasjpfan left a comment •

edited

Loading

glemaitre commented Jul 29, 2021 •

edited

Loading