Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

[MRG+2] Enable grid search with classifiers that may fail on individual fits. #2795

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
6 participants

Supersedes #2587. The old pull request was so outdated (and the functionality affected was moved to different files) that I decided to re-do the work starting with a fresh checkout from master. Since this is technically a new branch, I'm opening a new pull request.

@jnothman jnothman and 1 other commented on an outdated diff Jan 25, 2014

sklearn/tests/test_grid_search.py
@@ -644,3 +644,79 @@ def test_grid_search_with_multioutput_data():
correct_score = est.score(X[test], y[test])
assert_almost_equal(correct_score,
cv_validation_scores[i])
+
+
+class FailingClassifier(BaseEstimator):
+ """ Classifier that raises a ValueError on fit() """
+
+ def __init__(self, parameter=None):
+ self.parameter = parameter
+
+ def fit(self, X, y=None):
+ raise ValueError("Failing classifier failed as requiered")
@jnothman

jnothman Jan 25, 2014

Owner

Instead do:

if self.parameter:
    raise ...
return self
@jnothman

jnothman Jan 25, 2014

Owner

"requiered" -> "required"

@romaniukm

romaniukm Jan 26, 2014

What does raise ... do?

@jnothman

jnothman Jan 26, 2014

Owner

I was writing shorthand. I meant that your test would be more robust if the classifier didn't always fail, so you can check it doesn't give zero score to all parameters on failure

Owner

jnothman commented Jan 25, 2014

I think fit_exceptions_to_warnings and fit_exception_score are very long names that are hard to remember (the missing s in the second one is extra tricky). What's wrong with on_error : float or 'error' (or on_fit_error if you must).

You might as well warn always; use a custom warning class and the user can ignore it with warnings.simplefilter.

@jnothman jnothman commented on an outdated diff Jan 25, 2014

sklearn/cross_validation.py
estimator.fit(X_train, y_train, **fit_params)
- test_score = _score(estimator, X_test, y_test, scorer)
- if return_train_score:
- train_score = _score(estimator, X_train, y_train, scorer)
+
+ except Exception as e:
+ if fit_exceptions_to_warnings:
+ test_score = fit_exception_score
+ if return_train_score:
+ train_score = fit_exception_score
+ warnings.warn("Classifier fit failed. The score on this train-test"
+ " partition will be set to zero. Details: " +
@jnothman

jnothman Jan 25, 2014

Owner

'zero' should be variable: format with %0.1f, and %r for the exception.

@jnothman jnothman commented on an outdated diff Jan 25, 2014

sklearn/tests/test_grid_search.py
+ # refit was done, then an exception would be raised on refit and not
+ # caught by grid_search (expected behavior), and this would cause an
+ # error in this test.
+ gs = GridSearchCV(clf, [{'parameter': [0, 1, 2]}],
+ scoring='accuracy', refit=False)
+ gs.fit(X, y)
+
+ # Ensure that grid scores were set to zero as required
+ assert all(np.all(this_score.cv_validation_scores == 0.)
+ for this_score in gs.grid_scores_)
+
+ # Ensure that a warning was raised
+ assert len(w) > 0
+
+
+def test_grid_search_failing_classifier_no_y():
@jnothman

jnothman Jan 25, 2014

Owner

I think a separate test for no y is unnecessary. If it really needs testing, you can use a for loop.

I made some changes according to the feedback received here.

Meanwhile, I'm also having problems with some tests failing but these tests seem to be unrelated to the code I changed:

======================================================================
FAIL: sklearn.feature_extraction.tests.test_image.test_connect_regions
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/vol/medic02/users/mpr06/anaconda/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/vol/medic02/users/mpr06/sklearn-dev/anaconda/github/scikit-learn/sklearn/feature_extraction/tests/test_image.py", line 63, in test_connect_regions
    assert_equal(ndimage.label(mask)[1], connected_components(graph)[0])
AssertionError: 777 != 767
    '777 != 767' = '%s != %s' % (safe_repr(777), safe_repr(767))
    '777 != 767' = self._formatMessage('777 != 767', '777 != 767')
>>  raise self.failureException('777 != 767')


======================================================================
FAIL: sklearn.feature_extraction.tests.test_image.test_connect_regions_with_grid
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/vol/medic02/users/mpr06/anaconda/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/vol/medic02/users/mpr06/sklearn-dev/anaconda/github/scikit-learn/sklearn/feature_extraction/tests/test_image.py", line 70, in test_connect_regions_with_grid
    assert_equal(ndimage.label(mask)[1], connected_components(graph)[0])
AssertionError: 777 != 767
    '777 != 767' = '%s != %s' % (safe_repr(777), safe_repr(767))
    '777 != 767' = self._formatMessage('777 != 767', '777 != 767')
>>  raise self.failureException('777 != 767')

@jnothman jnothman commented on an outdated diff Feb 3, 2014

sklearn/cross_validation.py
estimator.fit(X_train, y_train, **fit_params)
- test_score = _score(estimator, X_test, y_test, scorer)
- if return_train_score:
- train_score = _score(estimator, X_train, y_train, scorer)
+
+ except Exception as e:
+ if skip_exceptions:
+ test_score = exception_score
+ if return_train_score:
+ train_score = exception_score
+ warnings.warn("Classifier fit failed. The score on this train-test"
+ " partition will be set to %f. Details: \n%r" %
+ (exception_score, e), RuntimeWarning)
@jnothman

jnothman Feb 3, 2014

Owner

Please sub-class RuntimeWarning so that the user can easily filter these warnings.

@jnothman jnothman commented on an outdated diff Feb 3, 2014

sklearn/cross_validation.py
estimator.fit(X_train, y_train, **fit_params)
- test_score = _score(estimator, X_test, y_test, scorer)
- if return_train_score:
- train_score = _score(estimator, X_train, y_train, scorer)
+
+ except Exception as e:
+ if skip_exceptions:
+ test_score = exception_score
+ if return_train_score:
+ train_score = exception_score
+ warnings.warn("Classifier fit failed. The score on this train-test"
+ " partition will be set to %f. Details: \n%r" %
+ (exception_score, e), RuntimeWarning)
+
@jnothman

jnothman Feb 3, 2014

Owner

I think this would be neater without the blank line.

Owner

jnothman commented Feb 3, 2014

I'm still not convinced you need separate parameters 'skip_exceptions' and 'exception_score', but all in all this is looking good.

Does someone else want to pitch in? @ogrisel, you've mentioned failing CV before...

@jnothman, I subclassed RuntimeWarning as you suggested. Any more ideas for improvement?

Owner

GaelVaroquaux commented Feb 11, 2014

I'm still not convinced you need separate parameters 'skip_exceptions' and 'exception_score'

I agree. Less parameters is better.

Can you explain how I can do this with one parameter?

Owner

jnothman commented Feb 11, 2014

For example:

GridSearchCV(on_error='raise') will do what the code currently does: raise
the exception

GridSearchCV(on_error=0) will return 0 in place of the breaking fold.

This assumes no one uses a class label called 'raise', but that may be a
fair enough assumption.

On 12 February 2014 02:15, Michal Romaniuk notifications@github.com wrote:

Can you explain how I can do this with one parameter?

Reply to this email directly or view it on GitHubhttps://github.com/scikit-learn/scikit-learn/pull/2795#issuecomment-34763490
.

@jnothman @GaelVaroquaux I'm not too keen on having one parameter with a special value but I changed it as you requested. Anything else?

@GaelVaroquaux GaelVaroquaux and 1 other commented on an outdated diff Feb 13, 2014

sklearn/cross_validation.py
@@ -1142,6 +1147,10 @@ def _fit_and_score(estimator, X, y, scorer, train, test, verbose, parameters,
verbose : integer
The verbosity level.
+ on_error : numeric or 'raise'
@GaelVaroquaux

GaelVaroquaux Feb 13, 2014

Owner

I think that I would call this parameter 'error_score'. When set to 'raise' it would indeed raise the exception.

@jnothman : what do you think of the proposed name?

@romaniukm

romaniukm Feb 17, 2014

I don't mind what we call it.

@romaniukm

romaniukm Feb 26, 2014

I'm ok with error_score.

@GaelVaroquaux GaelVaroquaux commented on an outdated diff Feb 13, 2014

sklearn/cross_validation.py
@@ -1112,9 +1112,14 @@ def cross_val_score(estimator, X, y=None, scoring=None, cv=None, n_jobs=1,
return np.array(scores)[:, 0]
-def _fit_and_score(estimator, X, y, scorer, train, test, verbose, parameters,
- fit_params, return_train_score=False,
- return_parameters=False):
+class FitFailedWarning(RuntimeWarning):
+ def __init__(self, message):
+ super(FitFailedWarning, self).__init__(message)
@GaelVaroquaux

GaelVaroquaux Feb 13, 2014

Owner

You don't need to reimplement the init. You can just put 'pass' as the content of the class.

@GaelVaroquaux GaelVaroquaux and 2 others commented on an outdated diff Feb 13, 2014

sklearn/cross_validation.py
estimator.fit(X_train, y_train, **fit_params)
- test_score = _score(estimator, X_test, y_test, scorer)
- if return_train_score:
- train_score = _score(estimator, X_train, y_train, scorer)
+ except Exception as e:
+ if on_error != 'raise':
+ test_score = on_error
+ if return_train_score:
+ train_score = on_error
+ warnings.warn("Classifier fit failed. The score on this train-test"
+ " partition will be set to %f. Details: \n%r" %
+ (on_error, e), FitFailedWarning)
@GaelVaroquaux

GaelVaroquaux Feb 13, 2014

Owner

I think that it would be useful to raise this warning with stacklevel=2 (at least, maybe more).

@jnothman

jnothman Feb 13, 2014

Owner

I think stacklevel works poorly in practice. In this case, popping off stack levels means you'll get a line number in joblib, no?

@romaniukm

romaniukm Feb 14, 2014

What does stacklevel do?

@GaelVaroquaux

GaelVaroquaux Feb 14, 2014

Owner

I think stacklevel works poorly in practice.

Why?

In this case, popping off stack levels means you'll get a line number
in joblib, no?

Not only in joblib. Everywhere.

@jnothman

jnothman Feb 15, 2014

Owner

What I mean is that the stacklevel before this one will be joblib code, not scikit-learn code.

stacklevel works poorly in practice when your function is not called directly by user code. Since the nesting of functions can change in different calling contexts and over time with rewrites and refactorings, it ultimately needs to be a parameter to the function.

And it hardly helps trace the warning.

@romaniukm

romaniukm Feb 17, 2014

So should I add stacklevel=2 or not?

@romaniukm

romaniukm Feb 26, 2014

Any more comments on stacklevel? I'm still not sure if I should use it or not.

@GaelVaroquaux GaelVaroquaux and 3 others commented on an outdated diff Feb 13, 2014

sklearn/tests/test_grid_search.py
+ def fit(self, X, y=None):
+ if self.parameter == FailingClassifier.FAILING_PARAMETER:
+ raise ValueError("Failing classifier failed as required")
+
+ def predict(self, X):
+ return np.zeros(X.shape[0])
+
+
+def test_grid_search_failing_classifier():
+ """ GridSearchCV with a failing classifier catches the error, sets the
+ score to zero and raises a warning """
+
+ with warnings.catch_warnings(record=True) as w:
+
+ # Cause all warnings to always be triggered.
+ warnings.simplefilter("always")
@GaelVaroquaux

GaelVaroquaux Feb 13, 2014

Owner

We have found that this pattern to test for warnings being raised was too fragile, and we use sklearn.utils.assert_warns.

@romaniukm

romaniukm Feb 14, 2014

I can't find sklearn.utils.assert_warns. Which file is it in?

@jnothman

jnothman Feb 15, 2014

Owner

It should be sklearn.utils.testing.assert_warns

@romaniukm

romaniukm Feb 15, 2014

I just looked at it and this worries me:
# To remove when we support numpy 1.7
I'm working with numpy 1.7...
In case it's fine, how is assert_warns supposed to be used?

@arjoly

arjoly Feb 15, 2014

Owner

The assert_warns function have backported for user with numpy version < 1.7. This code will be removed when the minimal numpy required for scikit-learn will be 1.7 or higher.

@romaniukm

romaniukm Feb 17, 2014

I found assert_warns now, but how is it supposed to be used?

@GaelVaroquaux

GaelVaroquaux Feb 17, 2014

Owner

I found assert_warns now, but how is it supposed to be used?

As the docstring says: assert_warns(UserWarning, func, arg1, arg2,
kwarg1=foo...)

@jnothman

jnothman Feb 17, 2014

Owner

In this case it would be assert_warns(FitFailedWarning, gs.fit, X, y)

@romaniukm

romaniukm Feb 18, 2014

So instead of

with warnings.catch_warnings(record=True) as w:
    warnings.simplefilter("always")
    X, y = make_classification(n_samples=20, n_features=10, random_state=0)
    clf = FailingClassifier()
    gs = GridSearchCV(clf, [{'parameter': [0, 1, 2]}], scoring='accuracy',
                      refit=False, on_error=0.0)
    gs.fit(X, y)

I would just have the following?

X, y = make_classification(n_samples=20, n_features=10, random_state=0)
clf = FailingClassifier()
gs = GridSearchCV(clf, [{'parameter': [0, 1, 2]}], scoring='accuracy',
                  refit=False, on_error=0.0)
assert_warns(FitFailedWarning, gs.fit, X, y)

My question is more about the whole usage pattern, not just assert_warns itself.

@romaniukm

romaniukm Feb 26, 2014

Just asking again about the usage pattern. Is the one above correct?

@jnothman

jnothman Feb 26, 2014

Owner

Yes, that looks right.

@GaelVaroquaux GaelVaroquaux commented on an outdated diff Feb 13, 2014

sklearn/grid_search.py
@@ -497,6 +503,11 @@ class GridSearchCV(BaseSearchCV):
verbose : integer
Controls the verbosity: the higher, the more messages.
+ on_error : numeric or 'raise'
+ Unless set to 'raise', catch exceptions in estimator fitting, display
+ them as warnings and assign score = on_error if such exceptions occur.
@GaelVaroquaux

GaelVaroquaux Feb 13, 2014

Owner

Maybe you should point out that exceptions will not be caught during the refit.

Owner

GaelVaroquaux commented Feb 13, 2014

I am OK with the general lines of this PR. I made just a few minor comments, the only one that might be a bit subject to discussion is the proposed name change for the argument.

Also, could you rebase your branch on master: it is not mergeable in the current state.

Thanks a lot for your work.

Hi @GaelVaroquaux and @jnothman,

I'm back to working on this after a long-ish break. I made the requested changes but it looks like something went wrong when I was trying to figure out how to do the rebase. Can you tell me if it's ok, and if not then what I should do?

Coverage Status

Coverage remained the same when pulling 0d3c952 on romaniukm:gridsearch-failing-classifiers-2 into c49723d on scikit-learn:master.

Owner

jnothman commented Mar 24, 2014

I've not looked at the damage (though it looks like you've done a merge rather than a rebase), but you can just do the rebase again if it's not arduous:

$ git checkout master
$ git pull https://github.com/scikit-learn/scikit-learn master:master
$ git checkout gridsearch-failing-classifiers-2
$ git reset --hard b5e5c5d  # take us back to "Renamed on_error ..."
$ git rebase master
# fix any conflicts, followed by git add and git rebase --continue
$ git push --force https://github.com/romaniukm/scikit-learn gridsearch-failing-classifiers-2

Coverage Status

Coverage remained the same when pulling ae70cdf on romaniukm:gridsearch-failing-classifiers-2 into b4625c7 on scikit-learn:master.

Rebase done.

@jnothman jnothman commented on an outdated diff Mar 30, 2014

sklearn/cross_validation.py
-def _fit_and_score(estimator, X, y, scorer, train, test, verbose, parameters,
- fit_params, return_train_score=False,
- return_parameters=False):
+
+def _fit_and_score(estimator, X, y, scorer, train, test, verbose,
+ parameters, fit_params, return_train_score=False,
+ return_parameters=False, skip_exceptions=False,
@jnothman

jnothman Mar 30, 2014

Owner

You still have the skip_exceptions parameter though it does nothing.

@jnothman jnothman commented on an outdated diff Mar 30, 2014

sklearn/cross_validation.py
@@ -1141,6 +1146,13 @@ def _fit_and_score(estimator, X, y, scorer, train, test, verbose, parameters,
verbose : integer
The verbosity level.
+ skip_exceptions : boolean
@jnothman

jnothman Mar 30, 2014

Owner

Please remove its documentation too.

@jnothman jnothman commented on an outdated diff Mar 30, 2014

sklearn/cross_validation.py
@@ -1141,6 +1146,13 @@ def _fit_and_score(estimator, X, y, scorer, train, test, verbose, parameters,
verbose : integer
The verbosity level.
+ skip_exceptions : boolean
+ Catch exceptions in estimator fitting, display them as warnings and
+ assign score = exception_score if such exceptions occur.
+
+ exception_score : numeric
@jnothman

jnothman Mar 30, 2014

Owner

make this 'raise' or numeric and explain 'raise' in the documentation.

Owner

jnothman commented Mar 30, 2014

I'm not too keen on having one parameter with a special value but I changed it as you requested.

We have parameter overloading all over the place in scikit-learn. In some places it's arguably more problematic than here, where 'raise' simply can't be a valid score, and score is irrelevant if an error is to be raised.

Thanks @jnothman for reviewing my code. I corrected the mistakes you pointed out and I hope it's all well now.

@jnothman jnothman commented on an outdated diff Apr 1, 2014

sklearn/cross_validation.py
@@ -1141,6 +1145,11 @@ def _fit_and_score(estimator, X, y, scorer, train, test, verbose, parameters,
verbose : integer
The verbosity level.
+ error_score : numeric or 'raise'
@jnothman

jnothman Apr 1, 2014

Owner

Could you make this 'raise' (default) or numeric.

@jnothman jnothman commented on an outdated diff Apr 1, 2014

sklearn/cross_validation.py
@@ -1141,6 +1145,11 @@ def _fit_and_score(estimator, X, y, scorer, train, test, verbose, parameters,
verbose : integer
The verbosity level.
+ error_score : numeric or 'raise'
+ Value to assign to score if an error occurs in estimator fitting. If
+ set to 'raise', an error is raised. This does not affect refit, which
@jnothman

jnothman Apr 1, 2014

Owner

"an error" -> "the error"

@jnothman jnothman commented on an outdated diff Apr 1, 2014

sklearn/cross_validation.py
@@ -1141,6 +1145,11 @@ def _fit_and_score(estimator, X, y, scorer, train, test, verbose, parameters,
verbose : integer
The verbosity level.
+ error_score : numeric or 'raise'
+ Value to assign to score if an error occurs in estimator fitting. If
@jnothman

jnothman Apr 1, 2014

Owner

add ", along with issuing a FitFailedWarning" or something.

@jnothman jnothman and 1 other commented on an outdated diff Apr 1, 2014

sklearn/cross_validation.py
estimator.fit(X_train, y_train, **fit_params)
- test_score = _score(estimator, X_test, y_test, scorer)
- if return_train_score:
- train_score = _score(estimator, X_train, y_train, scorer)
+
+ except Exception as e:
+ if error_score != 'raise':
@jnothman

jnothman Apr 1, 2014

Owner

If you put the short "if error_score == 'raise': raise" condition first, this will be easier to read (mirroring a linguistic phenomenon that prefers short-to-long whose name I can't recall).

@jnothman jnothman commented on an outdated diff Apr 1, 2014

sklearn/grid_search.py
@@ -216,6 +216,10 @@ 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 : numeric or 'raise'
@jnothman

jnothman Apr 1, 2014

Owner

Please amend as above.

@jnothman jnothman commented on an outdated diff Apr 1, 2014

sklearn/grid_search.py
@@ -671,6 +690,11 @@ class RandomizedSearchCV(BaseSearchCV):
verbose : integer
Controls the verbosity: the higher, the more messages.
+ error_score : numeric or 'raise'
@jnothman

jnothman Apr 1, 2014

Owner

Please amend as above.

@jnothman jnothman and 1 other commented on an outdated diff Apr 1, 2014

sklearn/grid_search.py
@@ -595,6 +610,10 @@ def fit(self, X, y=None):
None for unsupervised learning.
"""
+ if params:
+ warnings.warn("Additional parameters to GridSearchCV are ignored!"
+ " The params argument will be removed in 0.15.",
@jnothman

jnothman Apr 1, 2014

Owner

"The params argument" is a bit awkward for arbitrary kwargs. Perhaps "and will result in an exception in the future."

@romaniukm

romaniukm Apr 1, 2014

I don't think this is my code but I can make this change. Can you say what you want the whole message to be?

@jnothman

jnothman Apr 1, 2014

Owner

Then you've not been rebased correctly: params has already been removed.

Owner

jnothman commented Apr 1, 2014

It doesn't appear that the default raise behaviour is tested. But let me know if it's just covered in previous tests.

Otherwise, this LGTM.

@jnothman jnothman changed the title from [WIP] Enable grid search with classifiers that may fail on individual fits. to [MRGq] Enable grid search with classifiers that may fail on individual fits. Apr 1, 2014

@jnothman jnothman changed the title from [MRGq] Enable grid search with classifiers that may fail on individual fits. to [MRG] Enable grid search with classifiers that may fail on individual fits. Apr 1, 2014

What do you mean by 'raise' not being tested? The test in
test_grid_search.py checks that warnings are raised.

On 01/04/14 07:10, jnothman wrote:

It doesn't appear that the default |raise| behaviour is tested. But let
me know if it's just covered in previous tests.

Otherwise, this LGTM.


Reply to this email directly or view it on GitHub
#2795 (comment).

Owner

jnothman commented Apr 1, 2014

The test in test_grid_search.py checks that warnings are raised.

Yes, I meant a test to check that an error is raised if error_score='raise'. Testing coverage suggests it must already be covered by a test, but I couldn't easily work out which one.

Coverage Status

Coverage remained the same when pulling 0b7b7b9 on romaniukm:gridsearch-failing-classifiers-2 into fbe974b on scikit-learn:master.

Does assert_raises ensure that an error is raised and not just a warning?

I added an extra test, but I'm not sure if assert_raises will ensure that an error is raised (not just a warning).

Owner

jnothman commented Apr 7, 2014

Yes, assert_raises tests for an error being raised.

On 8 April 2014 03:10, Michal Romaniuk notifications@github.com wrote:

I added an extra test, but I'm not sure if assert_raises will ensure
that an error is raised (not just a warning).

Reply to this email directly or view it on GitHubhttps://github.com/scikit-learn/scikit-learn/pull/2795#issuecomment-39756673
.

@jnothman jnothman commented on the diff Apr 7, 2014

sklearn/tests/test_grid_search.py
+ # to individual folds will be caught and warnings raised instead. If
+ # refit was done, then an exception would be raised on refit and not
+ # caught by grid_search (expected behavior), and this would cause an
+ # error in this test.
+ gs = GridSearchCV(clf, [{'parameter': [0, 1, 2]}], scoring='accuracy',
+ refit=False, error_score=0.0)
+
+ assert_warns(FitFailedWarning, gs.fit, X, y)
+
+ # Ensure that grid scores were set to zero as required for those fits
+ # that are expected to fail
+ assert all(np.all(this_point.cv_validation_scores == 0.0)
+ for this_point in gs.grid_scores_
+ if this_point.parameters['parameter'] ==
+ FailingClassifier.FAILING_PARAMETER)
+
@jnothman

jnothman Apr 7, 2014

Owner

Please insert a second blank line here

@jnothman jnothman commented on an outdated diff Apr 7, 2014

sklearn/tests/test_grid_search.py
+
+ FAILING_PARAMETER = 2
+
+ def __init__(self, parameter=None):
+ self.parameter = parameter
+
+ def fit(self, X, y=None):
+ if self.parameter == FailingClassifier.FAILING_PARAMETER:
+ raise ValueError("Failing classifier failed as required")
+
+ def predict(self, X):
+ return np.zeros(X.shape[0])
+
+
+def test_grid_search_failing_classifier():
+ """ GridSearchCV with on_error != 'raise' and a failing classifier catches
@jnothman

jnothman Apr 7, 2014

Owner

The first line of a docstring should be a short summary. So you could change this to:

    """Test grid search with on_error != 'raise'

    Ensures that a warning is raised and score reset where appropriate.
    """

@jnothman jnothman commented on an outdated diff Apr 7, 2014

sklearn/tests/test_grid_search.py
+ FailingClassifier.FAILING_PARAMETER)
+
+def test_grid_search_failing_classifier_raise():
+ """ GridSearchCV with on_error == 'raise' raises the error """
+
+ X, y = make_classification(n_samples=20, n_features=10, random_state=0)
+
+ clf = FailingClassifier()
+
+ # refit=False because we want to test the behaviour of the grid search part.
+ gs = GridSearchCV(clf, [{'parameter': [0, 1, 2]}], scoring='accuracy',
+ refit=False, error_score='raise')
+
+ # FailingClassifier issues a ValueError so this is what we look for.
+ assert_raises(ValueError, gs.fit, X, y)
+
@jnothman

jnothman Apr 7, 2014

Owner

Please remove the blank line at end of file

@jnothman jnothman commented on an outdated diff Apr 7, 2014

sklearn/tests/test_grid_search.py
+
+ # Ensure that grid scores were set to zero as required for those fits
+ # that are expected to fail
+ assert all(np.all(this_point.cv_validation_scores == 0.0)
+ for this_point in gs.grid_scores_
+ if this_point.parameters['parameter'] ==
+ FailingClassifier.FAILING_PARAMETER)
+
+def test_grid_search_failing_classifier_raise():
+ """ GridSearchCV with on_error == 'raise' raises the error """
+
+ X, y = make_classification(n_samples=20, n_features=10, random_state=0)
+
+ clf = FailingClassifier()
+
+ # refit=False because we want to test the behaviour of the grid search part.
@jnothman

jnothman Apr 7, 2014

Owner

PEP8 wants a line width of <=79 characters. I count this as 80. Please drop the final .

Owner

jnothman commented Apr 7, 2014

Apart from those PEP8 things, LGTM! Thanks for all your effort, @romaniukm!

@jnothman jnothman changed the title from [MRG] Enable grid search with classifiers that may fail on individual fits. to [MRG+1] Enable grid search with classifiers that may fail on individual fits. Apr 7, 2014

Fixed the PEP8 issues. Thank you all for your help and patience!

Coverage Status

Coverage remained the same when pulling 78ce30e on romaniukm:gridsearch-failing-classifiers-2 into fbe974b on scikit-learn:master.

Owner

jnothman commented Apr 9, 2014

Would you also be able to add an entry in doc/whats_new.rst? Thanks.

Added an entry as requested.

If it's fine now, please merge it to avoid diverging from master again.

Owner

jnothman commented Apr 16, 2014

Could we please get another review? @GaelVaroquaux?

@jnothman, @gaelvarorquaux, what are the plans as to what to do next with this pull request?

Owner

jnothman commented May 6, 2014

You misspelled, @GaelVaroquaux... I've given this PR +1. Gael has to review the latest changes, or someone else review the whole PR, and approve it too. Then it will be merged.

You need to rebase it on master (or the person merging it does): one of the files you've touched has changed and an automatic merge isn't possible.

@GaelVaroquaux GaelVaroquaux commented on the diff May 7, 2014

sklearn/cross_validation.py
@@ -1141,6 +1145,12 @@ def _fit_and_score(estimator, X, y, scorer, train, test, verbose, parameters,
verbose : integer
The verbosity level.
+ error_score : 'raise' (default) or numeric
+ 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,
@GaelVaroquaux

GaelVaroquaux May 7, 2014

Owner

What does the numerical value mean? Why a numerical value?

@jnothman

jnothman May 7, 2014

Owner

This is stated on the line before, Value to assign to the score if an error occurs in estimator fitting.

But yes, the ordering makes it a little unclear. Perhaps "otherwise" would suffice. I wish there were also a better word than "raise" for warnings.

@GaelVaroquaux

GaelVaroquaux May 7, 2014

Owner

OK, maybe change the ordering of the lines in the docstring. But feel free to ignore this comment.

Owner

GaelVaroquaux commented May 7, 2014

@romaniukm : sorry for the slow reply, it's just that I am swamped. The amount of activity that scikit-learn pull requests create is crazy, and they add to my day work.

@GaelVaroquaux GaelVaroquaux commented on the diff May 7, 2014

sklearn/cross_validation.py
@@ -1192,13 +1202,25 @@ def _fit_and_score(estimator, X, y, scorer, train, test, verbose, parameters,
X_train, y_train = _safe_split(estimator, X, y, train)
X_test, y_test = _safe_split(estimator, X, y, test, train)
- if y_train is None:
- estimator.fit(X_train, **fit_params)
@GaelVaroquaux

GaelVaroquaux May 7, 2014

Owner

You've lost the lines calling the estimator without y_train when y was None. Do we have a clear view of the consequences? If not, I'd rather keep them.

@jnothman

jnothman May 7, 2014

Owner

Yes, this is fair enough: we can't be sure that all unsupervised downstream estimators have a second argument.

@romaniukm

romaniukm May 26, 2014

You mean to replace this with the following?

        if y_train is None:
            estimator.fit(X_train, **fit_params)
        else:
            estimator.fit(X_train, y_train, **fit_params)

@GaelVaroquaux GaelVaroquaux and 1 other commented on an outdated diff May 7, 2014

sklearn/cross_validation.py
estimator.fit(X_train, y_train, **fit_params)
- test_score = _score(estimator, X_test, y_test, scorer)
- if return_train_score:
- train_score = _score(estimator, X_train, y_train, scorer)
+
+ except Exception as e:
+ if error_score == 'raise':
+ raise
+ else:
@GaelVaroquaux

GaelVaroquaux May 7, 2014

Owner

I am worried that misspelling 'raise' will lead to incomprehensible error message. I'd like the 'else' clause transformed into an 'elif' clause that would be a bit more specific, and an 'else' clause raising an error.

@romaniukm

romaniukm May 26, 2014

Good point, I'll fix that.

Owner

GaelVaroquaux commented May 7, 2014

OK, aside the two comments above (y_train being none and elif clause) this is good to go as far as I am concerned. Good job!

Coverage Status

Coverage increased (+0.0%) when pulling 60ceb95 on romaniukm:gridsearch-failing-classifiers-2 into d0f6052 on scikit-learn:master.

@GaelVaroquaux I rebased and made the changes as requested.

I also decided to do some blatant self-promotion by adding my name to the contributors list in whats_new.rst ... I'm not sure though if I contributed enough so it's up to you to leave it or delete it.

Thanks for the feedback!

The Travis CI build seems to be failing but only in the configuration DISTRIB="ubuntu" PYTHON_VERSION="2.7" INSTALL_ATLAS="true" COVERAGE="true". Is there a way to see what went wrong with that build?

On my machine, two tests fail but I didn't change those files and they seem to be completely unrelated to my code:

======================================================================
FAIL: sklearn.feature_extraction.tests.test_image.test_connect_regions
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/vol/medic02/users/mpr06/anaconda/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/vol/medic02/users/mpr06/sklearn-dev/anaconda/github/scikit-learn/sklearn/feature_extraction/tests/test_image.py", line 63, in test_connect_regions
    assert_equal(ndimage.label(mask)[1], connected_components(graph)[0])
AssertionError: 777 != 767
    '777 != 767' = '%s != %s' % (safe_repr(777), safe_repr(767))
    '777 != 767' = self._formatMessage('777 != 767', '777 != 767')
>>  raise self.failureException('777 != 767')

======================================================================
FAIL: sklearn.feature_extraction.tests.test_image.test_connect_regions_with_grid
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/vol/medic02/users/mpr06/anaconda/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/vol/medic02/users/mpr06/sklearn-dev/anaconda/github/scikit-learn/sklearn/feature_extraction/tests/test_image.py", line 70, in test_connect_regions_with_grid
    assert_equal(ndimage.label(mask)[1], connected_components(graph)[0])
AssertionError: 777 != 767
    '777 != 767' = '%s != %s' % (safe_repr(777), safe_repr(767))
    '777 != 767' = self._formatMessage('777 != 767', '777 != 767')
>>  raise self.failureException('777 != 767')
Owner

ogrisel commented Jun 6, 2014

Can you please try to rebase your whole branch on top of the current master. I think those failures have been fixed there in the mean time.

Coverage Status

Coverage decreased (-0.0%) when pulling 01392cb on romaniukm:gridsearch-failing-classifiers-2 into b16ffbb on scikit-learn:master.

@ogrisel So I rebased and the Travis build passed this time, but those two tests still fail on my machine... Do you know where I can find some information about what causes this problem?

Does [MRG+1] mean it's planned to be merged in a future release?

Owner

jnothman commented Jun 11, 2014

MRG+1 means one reviewer has said this should be accepted. Generally, a PR requires a second +1 before merging.

@ogrisel ogrisel commented on an outdated diff Aug 1, 2014

sklearn/tests/test_grid_search.py
@@ -658,13 +659,60 @@ def test_predict_proba_disabled():
assert_false(hasattr(gs, "predict_proba"))
-def test_grid_search_allows_nans():
- """ Test GridSearchCV with Imputer """
- X = np.arange(20, dtype=np.float64).reshape(5, -1)
- X[2, :] = np.nan
- y = [0, 0, 1, 1, 1]
- p = Pipeline([
- ('imputer', Imputer(strategy='mean', missing_values='NaN')),
- ('classifier', MockClassifier()),
- ])
- gs = GridSearchCV(p, {'classifier__foo_param': [1, 2, 3]}, cv=2).fit(X, y)
+class FailingClassifier(BaseEstimator):
+ """ Classifier that raises a ValueError on fit() """
@ogrisel

ogrisel Aug 1, 2014

Owner

cosmetics: please remove the whitespaces at the beginning and the end of the docstring to be consistent with the PEP 257 style.

@ogrisel ogrisel commented on an outdated diff Aug 1, 2014

sklearn/tests/test_grid_search.py
+
+ FAILING_PARAMETER = 2
+
+ def __init__(self, parameter=None):
+ self.parameter = parameter
+
+ def fit(self, X, y=None):
+ if self.parameter == FailingClassifier.FAILING_PARAMETER:
+ raise ValueError("Failing classifier failed as required")
+
+ def predict(self, X):
+ return np.zeros(X.shape[0])
+
+
+def test_grid_search_failing_classifier():
+ """ GridSearchCV with on_error != 'raise'
@ogrisel

ogrisel Aug 1, 2014

Owner

same here

@ogrisel ogrisel commented on an outdated diff Aug 1, 2014

sklearn/tests/test_grid_search.py
+ # error in this test.
+ gs = GridSearchCV(clf, [{'parameter': [0, 1, 2]}], scoring='accuracy',
+ refit=False, error_score=0.0)
+
+ assert_warns(FitFailedWarning, gs.fit, X, y)
+
+ # Ensure that grid scores were set to zero as required for those fits
+ # that are expected to fail.
+ assert all(np.all(this_point.cv_validation_scores == 0.0)
+ for this_point in gs.grid_scores_
+ if this_point.parameters['parameter'] ==
+ FailingClassifier.FAILING_PARAMETER)
+
+
+def test_grid_search_failing_classifier_raise():
+ """ GridSearchCV with on_error == 'raise' raises the error """
@ogrisel

ogrisel Aug 1, 2014

Owner

and here again.

Owner

ogrisel commented Aug 1, 2014

Please update the what's new file to move that change to the block of the 0.16 release (you might need to rebase again on current master first).

Also please squash this PR as a single commit.

Then +1 for merge on my side as well.

Owner

ogrisel commented Aug 1, 2014

git fetch upstream
git rebase -i upstream/master

pick the first commit and squash the other and put a descriptive commit message.

then git push -f romaniukm gridsearch-failing-classifiers-2

Coverage Status

Coverage decreased (-0.0%) when pulling b29a54c on romaniukm:gridsearch-failing-classifiers-2 into 0a7bef6 on scikit-learn:master.

@jnothman I'm not sure how your DOC commit ended up in my local master but not the github one... Was it rolled back? In that case, what should I do about it?

Coverage Status

Coverage decreased (-0.0%) when pulling bf8c7b6 on romaniukm:gridsearch-failing-classifiers-2 into 22cafa6 on scikit-learn:master.

Owner

jnothman commented Aug 5, 2014

Somehow you have cherry-picked in that commit (or based on-top of it); in master it includes a merge commit that appears to be absent here. You should be able to do a rebase on an updated master, but don't worry about it too much

It looks like this branch went out of sync with master again...

Owner

jnothman commented Oct 1, 2014

Okay. This PR has 3 +1s, and the title should reflect this... I'll try to rebase and merge today.

@jnothman jnothman changed the title from [MRG+1] Enable grid search with classifiers that may fail on individual fits. to [MRG+2] Enable grid search with classifiers that may fail on individual fits. Oct 1, 2014

Owner

jnothman commented Oct 2, 2014

Merged as 58be184. Thanks @romaniukm (and for your patience!)

@jnothman jnothman closed this Oct 2, 2014

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