Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRG+1] Fixed SelectKBest corner case: k=0 #3010

Merged
merged 1 commit into from Mar 27, 2014

Conversation

griffinmyers
Copy link

Without this fix k = 0 will have the same behavior as k = 'all' based on how
the array indexing was written.

I've included a test that demonstrates proper behaviour.

Without this fix k = 0 will have the same behavior as k = 'all' based on how
the array indexing was written.

I've included a test that demonstrates proper behaviour.
@GaelVaroquaux
Copy link
Member

LGTM. 👍 for merge. Thanks!

@GaelVaroquaux GaelVaroquaux changed the title Fixed SelectKBest corner case: k=0 [MRG+1] Fixed SelectKBest corner case: k=0 Mar 27, 2014
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling f1e2e1c on griffinmyers:fix-select-k-best into eb10c4c on scikit-learn:master.

@jnothman
Copy link
Member

This looks fine, but I wonder why an error would not be appropriate?

@griffinmyers
Copy link
Author

In my case k=0 is a useful argument. Imagine you have a FeatureUnion of two Pipelines, each with its own selector. I'd like to be able to do a cross_val_score run where one of the pipelines produces zero features.

@jnothman
Copy link
Member

Yes, I thought that might be the case. The ability to switch off a component in a FeatureUnion or Pipeline was mooted about a year ago (#1769).

But SelectPercentile(percentile=0) is explicitly supported, so I guess yours is a correct solution in any case.

jnothman added a commit that referenced this pull request Mar 27, 2014
@jnothman jnothman merged commit a3f81d9 into scikit-learn:master Mar 27, 2014
@jnothman
Copy link
Member

Thanks! merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants