Repository: scikit-learn/scikit-learn
Pull request: [MRG+1] Fixes #10310 Cross Validation FitFailedWarning message and exception
<!--
Thanks for contributing a pull request! Please ensure you have taken a look at
the contribution guidelines: https://github.com/scikit-learn/scikit-learn/blob/master/CONTRIBUTING.md#pull-request-checklist
-->

#### Reference Issues/PRs
fixes #10310 

<!--
Example: Fixes #1234. See also #3456.
Please use keywords (e.g., Fixes) to create link to the issues or pull requests
you resolved, so that they will automatically be closed when your pull request
is merged. See https://github.com/blog/1506-closing-issues-via-pull-requests
-->


#### What does this implement/fix? Explain your changes.

- [x] Changed 'Classifier' to 'Estimator' in the warning message

- [x] Better formatting of exception using format_exception_only

- [x] Test case to test the message

#### Any other comments?

I previously tried to fix the same issue in #10313 but unfortunately I was fixing the wrong file. Instead of updating on the same thread I closed that PR and opened this one so it would be easier for the reviewers. 

<!--
Please be aware that we are a loose team of volunteers so patience is
necessary; assistance handling other issues is very welcome. We value
all user contributions, no matter how minor they are. If we are slow to
review, either the pull request needs some benchmarking, tinkering,
convincing, etc. or more likely the reviewers are simply busy. In either
case, we ask for your understanding during the review process.
For more information, see our FAQ on this topic:
http://scikit-learn.org/dev/faq.html#why-is-my-pull-request-not-getting-any-attention.

Thanks for contributing!
-->

  
  

Problem statement:
FitFailedWarning raised by cross validation could do with better message
Current message says "Classifier fit failed" but the estimator is not necessarily a classifier. (Perhaps that's too pedantic of me...)

`%r` is not technically the best way to display an error message. We could either use `traceback.format_exc` and include the whole traceback; or we can use `traceback.format_exception_only` to print it properly (though I think this is the same as `"%s: %s" % (type(exc), exc)`).

Another thing we can consider, now that `_fit_and_score` provides structured results to `*SearchCV` and `cross_validate`, is to store the full traceback in some array of `*SearchCV.cv_results_`.

Hints from issue discussion:
I've attempted to fix it, please check.

Commit message: Fixes #10310: Cross Validation FitFailedWarning message and exception formatting

Patch to review:
diff --git a/sklearn/model_selection/_validation.py b/sklearn/model_selection/_validation.py
index 24362620c809f..d6d4c0924b350 100644
--- a/sklearn/model_selection/_validation.py
+++ b/sklearn/model_selection/_validation.py
@@ -15,6 +15,7 @@
 import warnings
 import numbers
 import time
+from traceback import format_exception_only
 
 import numpy as np
 import scipy.sparse as sp
@@ -474,9 +475,11 @@ def _fit_and_score(estimator, X, y, scorer, train, test, verbose,
                 test_scores = error_score
                 if return_train_score:
                     train_scores = error_score
-            warnings.warn("Classifier fit failed. The score on this train-test"
+            warnings.warn("Estimator fit failed. The score on this train-test"
                           " partition for these parameters will be set to %f. "
-                          "Details: \n%r" % (error_score, e), FitFailedWarning)
+                          "Details: \n%s" %
+                          (error_score, format_exception_only(type(e), e)[0]),
+                          FitFailedWarning)
         else:
             raise ValueError("error_score must be the string 'raise' or a"
                              " numeric value. (Hint: if using 'raise', please"
diff --git a/sklearn/model_selection/tests/test_validation.py b/sklearn/model_selection/tests/test_validation.py
index ad49385100491..2fa01c0a5fb3d 100644
--- a/sklearn/model_selection/tests/test_validation.py
+++ b/sklearn/model_selection/tests/test_validation.py
@@ -1421,3 +1421,31 @@ def test_permutation_test_score_pandas():
         check_series = lambda x: isinstance(x, TargetType)
         clf = CheckingClassifier(check_X=check_df, check_y=check_series)
         permutation_test_score(clf, X_df, y_ser)
+
+
+def test_fit_and_score():
+    from sklearn.tests.test_grid_search import FailingClassifier
+    from sklearn.model_selection._validation import _fit_and_score
+    from sklearn.exceptions import FitFailedWarning
+
+    # Create a failing classifier to deliberately fail
+    failing_clf = FailingClassifier(FailingClassifier.FAILING_PARAMETER)
+    # dummy X data
+    X = np.arange(1, 10)
+    fit_and_score_args = [failing_clf, X, None, dict(), None,
+                          None, None, None, None]
+    # passing error score to trigger the warning message
+    fit_and_score_kwargs = {'error_score': 0}
+    # check if the warning message type is as expected
+    assert_warns(FitFailedWarning, _fit_and_score, *fit_and_score_args,
+                 **fit_and_score_kwargs)
+    # since we're using FailingClassfier, our error will be the following
+    error_message = "ValueError: Failing classifier failed as required"
+    # the warning message we're expecting to see
+    warning_message = "Estimator fit failed. The score on this train-test" \
+                      " partition for these parameters will be set to %f. " \
+                      "Details: \n%s" % (fit_and_score_kwargs['error_score'],
+                                         error_message)
+    # check if the same warning is triggered
+    assert_warns_message(FitFailedWarning, warning_message, _fit_and_score,
+                         *fit_and_score_args, **fit_and_score_kwargs)


Code review comments:
diff_hunk: @@ -1421,3 +1421,31 @@ def test_permutation_test_score_pandas():
         check_series = lambda x: isinstance(x, TargetType)
         clf = CheckingClassifier(check_X=check_df, check_y=check_series)
         permutation_test_score(clf, X_df, y_ser)
+
+
+def test_fit_and_score():
+    from sklearn.tests.test_grid_search import FailingClassifier
+    from sklearn.model_selection._validation import _fit_and_score
path: sklearn/model_selection/tests/test_validation.py
text: @author:
While I know it's a bad practice to import the protected method _fit_and_score, the required warning message would be raised only by passing the error_score parameter which I couldn't do with `cross_val_score` or `cross_validate` public functions without adding an additional parameter (and I don't know if it'd be great to add a  parameter for this trivial purpose) 

@user1:
Well `GridSearchCV(est,{},refit=False).fit(X,y)` is basically the same as `cross_val_score`...

But I think this is perfectly fine in unit testing. Still not sure why you need to import it locally, not at the top of the file.

@author:
@user1 I've fixed it. But the Travis CI build fails with a weird error. What do you suggest?