[MRG+1] explicit exception message for strict selectors #4206

ogrisel · 2015-02-05T16:23:51Z

This is a fix for #4059 to raise a ValueError at transform time with an explicit error message instead of crashing when calling get_support() with a cryptic error message.

Note that for consistency, the behavior of SelectKBest(k=0) is also impacted by this change.

coveralls · 2015-02-05T16:34:50Z

Coverage increased (+0.0%) to 95.0% when pulling 99f0044 on ogrisel:fix-strict-select-fdr into 1d08f08 on scikit-learn:master.

jnothman · 2015-02-05T21:47:23Z

FWIW, I can imagine cases in which SelectKBest(k=0) is used, in the context of a grid search over FeatureUnion.

amueller · 2015-02-05T23:05:24Z

LGTM

jnothman · 2015-02-05T23:39:03Z

In case some user code depends on the prior behaviour of SelectKBest and SelectPercentile -- which explicitly says percentile >= 0, I would rather raise a warning for now, perhaps saying "this will become an error in the future. Report to the scikit-learn mailing list if this will break your usage."

But this is more reasonable given the error_score feature in GridSearchCV.

amueller · 2015-02-05T23:52:20Z

@jnothman in which case does that raise an error that didn't raise an error before?

jnothman · 2015-02-06T00:22:08Z

SelectPercentile(percentile=0).fit_transform([[0]], [0]) raises an error with this PR

ogrisel · 2015-02-06T08:59:54Z

But passing an empty data array as input to the next step in the pipeline is extremely likely to cause hard to diagnose errors. I think it's better to fail fast with an explicit error message at the point where the problem is fixable.

I you find one legitimate case to transform data into an empty dataset I will reconsider this by ATM I don't see any.

ogrisel · 2015-02-06T09:04:15Z

This is the kind of weird downstream errors that might be caused by passing empty datasets to models:

>>> X = np.ones(0).reshape(10, 0)
>>> y = np.random.random_integers(0, 2, 10)
>>> SVC().fit(X, y)
Traceback (most recent call last):
  File "<ipython-input-20-0c9fbc689f4e>", line 1, in <module>
    SVC().fit(X, y)
  File "/Users/ogrisel/code/scikit-learn/sklearn/svm/base.py", line 164, in fit
    self._gamma = 1.0 / X.shape[1]
ZeroDivisionError: float division by zero

We should probably make our input validation check stricter for the case where X.shape[1] == 0 to raise better error messages in general but It think it's worth fixing it upstream in the selector mixin as well.

amueller · 2015-02-06T10:13:06Z

The use case @jnothman mentioned is where you want to grid-search over disabling one part of a FeatureUnion basically. That is somewhat vaild, and I feel we might not want to break it.

Two ways out:

Deprecate the percentile=0 case and possibly enrage users that used this
Return an empty array and warn that we returned no features, possibly leading to weird error messages (or fix the error message everywhere, which is an additional line in check_array ;) )

(FYI @jnothman's example gives a zero division warning on master)

ogrisel · 2015-02-06T10:44:13Z

Thanks for the clarification. I now get the FeatureUnion use case... I find it ugly to just issue a warning but maybe this is the best we can do in that case.

fix the error message everywhere, which is an additional line in check_array

This is good independently of the rest of the discussion, I will work on a PR for that.

amueller · 2015-02-06T10:46:05Z

Thanks @ogrisel :) Btw there are a couple more bug-fix PRs that could use your reviews ;) https://github.com/scikit-learn/scikit-learn/pulls?q=is%3Aopen+is%3Apr+milestone%3A0.16

ogrisel · 2015-02-06T11:09:28Z

I updated this PR based on your feedback to issue a UserWarning instead of raising a ValueError. Would you like me to introduce a new EmptyDataWarning class as a subclass of UserWarning to make it easier for users to silence this specific class of warnings when running grid search with FeatureUnion?

ogrisel · 2015-02-06T11:10:17Z

Thanks @ogrisel :) Btw there are a couple more bug-fix PRs that could use your reviews ;)

I know. I plan to dedicate this day to them (and maybe the BH t-SNE PR that deserves some love too).

ogrisel · 2015-02-06T11:12:53Z

BTW @amueller feel free to join http://gitter.im/scikit-learn/scikit-learn to speak about the priority of the bug fix issues to review.

amueller · 2015-02-06T11:13:08Z

Thanks and sorry for pestering ;) I'm travelling this week but will be more (re)active again next week.

coveralls · 2015-02-06T11:18:57Z

Coverage increased (+0.0%) to 95.0% when pulling bc5b403 on ogrisel:fix-strict-select-fdr into a168a6c on scikit-learn:master.

ogrisel · 2015-02-06T14:19:00Z

@amueller @jnothman would you prefer this to issue a specific new class EmptyDataWarning(UserWarning) or do you prefer to keep it like that with a generic UserWarning? I don't have strong opinion. I would like to make it easy to silence specific warnings but I don't want to introduce warning types bloat.

amueller · 2015-02-06T14:24:01Z

I'm not a huge fan of creating new warnings in most situations, -0 from me ;)

ogrisel · 2015-02-06T14:42:51Z

@amueller I take your thanks as a +1 and marked this in the title to attract more reviewers, feel free to re-edit the title if you have more comments or don't think this is ready for merge.

amueller · 2015-02-07T08:08:59Z

sklearn/feature_selection/tests/test_feature_select.py

+    # rejects all the features
+    X = rng.rand(40, 10)
+    y = rng.randint(0, 4, size=40)
+    fdr = SelectFdr(alpha=0.00001).fit(X, y)


Maybe you should test all heuristics and also SelectKBest and SelectPercentile?

coveralls · 2015-02-07T10:45:05Z

Coverage increased (+0.01%) to 95.02% when pulling 2ce9eac on ogrisel:fix-strict-select-fdr into a168a6c on scikit-learn:master.

jnothman · 2015-02-07T10:52:50Z

Regarding the FeatureUnion case, I might remind that #1769 is open and provides an alternative means of disabling an element of a FeatureUnion (or Pipeline). Of course that won't be a recourse for users in the short-term (and remains syntactically clunky when using GridSearch in that it's not just an alternative value of k).

And this LGTM. Merging.

jnothman · 2015-02-07T10:54:22Z

After saying LGTM, I've realised LearntSelectorMixin, etc aren't tested. Still, this isn't a real impediment.

[MRG] explicit warning message for strict selectors Also fixes #4059

ogrisel mentioned this pull request Feb 5, 2015

Crash in univariate feature selection if no feature is selected. #4059

Closed

FIX scikit-learn#4059: explicit warning for strict selectors

bc5b403

ogrisel force-pushed the fix-strict-select-fdr branch from 99f0044 to bc5b403 Compare February 6, 2015 11:07

ogrisel changed the title ~~[MRG] explicit exception message for strict selectors~~ [MRG+1] explicit exception message for strict selectors Feb 6, 2015

ogrisel added the Bug label Feb 6, 2015

ogrisel added this to the 0.16 milestone Feb 6, 2015

ogrisel mentioned this pull request Feb 6, 2015

[MRG+2] add validation for non-empty input data #4214

Merged

amueller reviewed Feb 7, 2015
View reviewed changes

TST more strict selectors with empty support

2ce9eac

jnothman added a commit that referenced this pull request Feb 7, 2015

Merge pull request #4206 from ogrisel/fix-strict-select-fdr

95681ee

[MRG] explicit warning message for strict selectors Also fixes #4059

jnothman merged commit 95681ee into scikit-learn:master Feb 7, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG+1] explicit exception message for strict selectors #4206

[MRG+1] explicit exception message for strict selectors #4206

[MRG+1] explicit exception message for strict selectors #4206

[MRG+1] explicit exception message for strict selectors #4206

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment