[go: nahoru, domu]

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] API use drop a sentinel to disable estimators in voting #13780

Merged
merged 17 commits into from
May 7, 2019

Conversation

glemaitre
Copy link
Member
@glemaitre glemaitre commented May 3, 2019

closes #13771

For consistency, we should allow 'drop' to work the same as None to disable estimators in Voting estimators.

Need to merge #13777 before to have the bug fix regarding the None and the sample weight.

@glemaitre glemaitre changed the title API use drop a sentinel to disable estimators in voting [MRG] API use drop a sentinel to disable estimators in voting May 3, 2019
Copy link
Member
@jnothman jnothman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise lgtm.

When was the None support added? I'd be tempted to merge this for 0.21 and not support None if that was only just added now...

# regression test for
# https://github.com/scikit-learn/scikit-learn/issues/13777
voter.fit(X, y, sample_weight=np.ones(y.shape))
voter.set_params(lr=None)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or drop?

return [w for est, w in zip(self.estimators,
self.weights) if est[1] is not None]
return [w for est, w in zip(self.estimators, self.weights)
if not(est[1] is None or est[1] == 'drop')]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use in (None, 'drop')?

@glemaitre
Copy link
Member Author

None was introduced in 0.19: #7674 I assume that we have to keep it then.

@glemaitre
Copy link
Member Author

@jnothman Do you want to merge this PR directly and I am closing #13777

I also incorporated the change required to avoid using has_fit_parameter. The only drawback is that it will fail a bit later in fit (meaning that we can spend time fitting some estimators before to fail).
WDYT?

@glemaitre
Copy link
Member Author

@qinhanmin2014 You can have a look a this now. I solved the conflicts.

Copy link
Member
@jnothman jnothman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM

sklearn/ensemble/tests/test_voting.py Show resolved Hide resolved
sklearn/ensemble/voting.py Outdated Show resolved Hide resolved
sklearn/ensemble/tests/test_voting.py Outdated Show resolved Hide resolved
sklearn/ensemble/voting.py Outdated Show resolved Hide resolved
sklearn/ensemble/voting.py Outdated Show resolved Hide resolved
Copy link
Member
@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thomasjpfan
Copy link
Member

@jnothman Good to have in 0.21?

@jnothman jnothman merged commit 1e2e847 into scikit-learn:master May 7, 2019
jnothman pushed a commit to jnothman/scikit-learn that referenced this pull request May 7, 2019
koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
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.

Handle 'drop' together with None to drop estimator in VotingClassifier/VotingRegressor
3 participants