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

romaniukm · 2014-01-25T21:09:40Z

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 · 2014-01-25T21:58:53Z

sklearn/tests/test_grid_search.py

+        self.parameter = parameter
+
+    def fit(self, X, y=None):
+        raise ValueError("Failing classifier failed as requiered")


Instead do:

if self.parameter: raise ... return self

"requiered" -> "required"

What does raise ... do?

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

jnothman · 2014-01-25T22:01:46Z

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 · 2014-01-25T22:24:24Z

sklearn/cross_validation.py

+            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: " +


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

romaniukm · 2014-01-28T15:48:33Z

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 · 2014-02-03T01:59:33Z

sklearn/cross_validation.py

+                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)


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

jnothman · 2014-02-03T02:01:06Z

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...

romaniukm · 2014-02-04T17:53:10Z

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

GaelVaroquaux · 2014-02-11T14:29:14Z

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

I agree. Less parameters is better.

romaniukm · 2014-02-11T15:15:23Z

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

jnothman · 2014-02-11T20:39:48Z

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//pull/2795#issuecomment-34763490
.

romaniukm · 2014-02-13T18:29:08Z

@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 · 2014-02-13T19:47:17Z

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'


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?

I don't mind what we call it.

I'm ok with error_score.

GaelVaroquaux · 2014-02-13T20:27:17Z

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.

romaniukm · 2014-03-24T18:14:11Z

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?

coveralls · 2014-03-24T20:38:51Z

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

jnothman · 2014-03-24T23:01:51Z

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

coveralls · 2014-03-26T19:20:35Z

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

romaniukm · 2014-03-28T18:45:30Z

Rebase done.

jnothman · 2014-03-30T06:17:55Z

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, skip_exceptions=False,


You still have the skip_exceptions parameter though it does nothing.

jnothman · 2014-03-30T06:20:17Z

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.

romaniukm · 2014-03-30T17:41:22Z

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

jnothman · 2014-04-01T05:58:59Z

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'


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

GaelVaroquaux · 2014-05-07T04:55:42Z

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,


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

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.

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

GaelVaroquaux · 2014-05-07T05:00:08Z

@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 · 2014-05-07T05:00:51Z

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)


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.

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

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 · 2014-05-07T05:50:05Z

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!

coveralls · 2014-05-26T16:14:12Z

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

romaniukm · 2014-05-26T18:28:04Z

@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!

romaniukm · 2014-05-27T19:56:22Z

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')

ogrisel · 2014-06-06T09:09:28Z

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.

coveralls · 2014-06-06T16:13:29Z

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

romaniukm · 2014-06-06T17:16:21Z

@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?

romaniukm · 2014-06-11T19:06:51Z

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

jnothman · 2014-06-11T23:34:34Z

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

ogrisel · 2014-08-01T11:55:34Z

sklearn/tests/test_grid_search.py

-    ])
-    gs = GridSearchCV(p, {'classifier__foo_param': [1, 2, 3]}, cv=2).fit(X, y)
+class FailingClassifier(BaseEstimator):
+    """ Classifier that raises a ValueError on fit() """


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

ogrisel · 2014-08-01T11:58:01Z

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.

ogrisel · 2014-08-01T11:59:48Z

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

coveralls · 2014-08-04T21:16:19Z

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

…dual fits.

romaniukm · 2014-08-05T18:05:44Z

@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?

coveralls · 2014-08-05T18:18:24Z

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

jnothman · 2014-08-05T22:09:53Z

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

romaniukm · 2014-09-16T10:46:51Z

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

jnothman · 2014-10-01T23:50:57Z

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

jnothman · 2014-10-02T00:04:01Z

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

jnothman reviewed Jan 25, 2014
View reviewed changes

jnothman reviewed Feb 3, 2014
View reviewed changes

GaelVaroquaux reviewed Feb 13, 2014
View reviewed changes

jnothman reviewed Mar 30, 2014
View reviewed changes

jnothman reviewed Apr 1, 2014
View reviewed changes

GaelVaroquaux reviewed May 7, 2014
View reviewed changes

ogrisel reviewed Aug 1, 2014
View reviewed changes

jnothman and others added 2 commits August 5, 2014 18:55

DOC A less-nested coverage of model evaluation

9b42e55

Enable grid search with classifiers that may throw an error on indivi…

bf8c7b6

…dual fits.

larsmans force-pushed the master branch from 58a55ad to 4b82379 Compare August 25, 2014 21:50

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

jnothman closed this Oct 2, 2014

Uh oh!

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

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

Uh oh!

Conversation

romaniukm commented Jan 25, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnothman commented Jan 25, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romaniukm commented Jan 28, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnothman commented Feb 3, 2014

Uh oh!

romaniukm commented Feb 4, 2014

Uh oh!

GaelVaroquaux commented Feb 11, 2014

Uh oh!

romaniukm commented Feb 11, 2014

Uh oh!

jnothman commented Feb 11, 2014

Uh oh!

romaniukm commented Feb 13, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GaelVaroquaux commented Feb 13, 2014

Uh oh!

romaniukm commented Mar 24, 2014

Uh oh!

coveralls commented Mar 24, 2014

Uh oh!

jnothman commented Mar 24, 2014

Uh oh!

coveralls commented Mar 26, 2014

Uh oh!

romaniukm commented Mar 28, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnothman commented Mar 30, 2014

Uh oh!

romaniukm commented Mar 30, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GaelVaroquaux commented May 7, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GaelVaroquaux commented May 7, 2014

Uh oh!

coveralls commented May 26, 2014

Uh oh!

romaniukm commented May 26, 2014