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] FIX run test for meta-estimator having estimators keyword #14305

Merged
merged 14 commits into from Jul 29, 2019

Conversation

glemaitre
Copy link
Contributor

@glemaitre glemaitre commented Jul 11, 2019

We should start to run tests for meta-estimator like VotingClassifier and VotingRegressor.

This is also useful for the stackingclassifier and stackingregressor for which we should ensure to pass these tests.

@glemaitre glemaitre changed the title [WIP] FIX run test for meta-estimator having estimators keyword [MRG] FIX run test for meta-estimator having estimators keyword Jul 11, 2019
@glemaitre
Copy link
Contributor Author

@glemaitre glemaitre commented Jul 11, 2019

I would be happy to have some feedback @amueller @jnothman @thomasjpfan @NicolasHug

sklearn/utils/estimator_checks.py Outdated Show resolved Hide resolved
sklearn/utils/estimator_checks.py Outdated Show resolved Hide resolved
sklearn/ensemble/voting.py Show resolved Hide resolved
Copy link
Member

@NicolasHug NicolasHug left a comment

Thanks for doing this, it was clearly lacking.

Only a few comments, looks good overall.

doc/whats_new/v0.22.rst Outdated Show resolved Hide resolved
@@ -84,6 +84,13 @@ Changelog
preserve the class balance of the original training set. :pr:`14194`
by :user:`Johann Faouzi <johannfaouzi>`.

- |Fix| Enable to run :func:`utils.check_estimator` on both
Copy link
Member

@NicolasHug NicolasHug Jul 18, 2019

Choose a reason for hiding this comment

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

since this is not in the utils module section, the link is broken ;)

sklearn/tests/test_common.py Outdated Show resolved Hide resolved
sklearn/utils/estimator_checks.py Outdated Show resolved Hide resolved
@@ -2165,6 +2173,21 @@ def check_parameters_default_constructible(name, Estimator):
estimator = Estimator(Ridge())
else:
estimator = Estimator(LinearDiscriminantAnalysis())
elif "estimators" in required_parameters:
Copy link
Member

@NicolasHug NicolasHug Jul 18, 2019

Choose a reason for hiding this comment

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

It seems that this whole estimator initialization is duplicated between check_parameters_default_constructible and _tested_estimators. Might be worth considering a unifying helper?

Copy link
Contributor Author

@glemaitre glemaitre Jul 26, 2019

Choose a reason for hiding this comment

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

I agree but I would do that in another PR or go for a better solution as proposed by @jnothman.

@jnothman
Copy link
Member

@jnothman jnothman commented Jul 25, 2019

I find this too implicit/magical. I'd really just rather a way for estimators to specify test parameters, which I was moving towards in #11324.

@glemaitre
Copy link
Contributor Author

@glemaitre glemaitre commented Jul 26, 2019

I find this too implicit/magical. I'd really just rather a way for estimators to specify test parameters, which I was moving towards in #11324.

I agree. Could we find a middle ground by introducing this test for the time being and later on refactorize/remove it by something as you are proposing. In the meantime, we at least run some tests on these estimators.

@NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Jul 26, 2019

I agree that we can temporarily merge this. It is indeed implicit etc., but after all that's what we've been doing with all the other meta-estimators so far. I'll try to take a look at #11324.

@glemaitre I think I can approve once you address the comments?

@glemaitre
Copy link
Contributor Author

@glemaitre glemaitre commented Jul 26, 2019

@NicolasHug I should have addressed all comments

Copy link
Member

@NicolasHug NicolasHug left a comment

Thanks @glemaitre

Copy link
Member

@amueller amueller left a comment

Looks mostly good, I'd really rather not add to the checking parameters though.

@@ -396,6 +399,10 @@ def set_checking_parameters(estimator):
if name == 'OneHotEncoder':
estimator.set_params(handle_unknown='ignore')

# set voting='soft' to be able to use predict_proba
if name == 'VotingClassifier':
estimator.set_params(voting='soft')
Copy link
Member

@amueller amueller Jul 26, 2019

Choose a reason for hiding this comment

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

Is this necessary to pass the test or do you just add it so we can test the version with predict_proba? I'd really rather not add anything here, and if we want to test this particular instantiation, we should call check_estimator on VotingClassifier with these parameters directly.

This should be necessary for making the tests pass if the ducktyping works correctly.

Copy link
Contributor Author

@glemaitre glemaitre Jul 26, 2019

Choose a reason for hiding this comment

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

This is necessary otherwise predict_proba is not defined and raise an error (leading to some failure in the common tests).

Copy link
Member

@amueller amueller Jul 26, 2019

Choose a reason for hiding this comment

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

#14287 might have fixed this?

Copy link
Contributor Author

@glemaitre glemaitre Jul 29, 2019

Choose a reason for hiding this comment

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

right

@@ -107,6 +107,13 @@ Changelog
preserve the class balance of the original training set. :pr:`14194`
by :user:`Johann Faouzi <johannfaouzi>`.

- |Fix| Enable to run :func:`utils.estimator_checks.check_estimator` on both
Copy link
Member

@amueller amueller Jul 26, 2019

Choose a reason for hiding this comment

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

I would say "run it by default" because you could already run it by giving it an instance.

@jnothman
Copy link
Member

@jnothman jnothman commented Jul 27, 2019

@glemaitre
Copy link
Contributor Author

@glemaitre glemaitre commented Jul 29, 2019

@jnothman I moved the tests in ensemble/tests/test_voting.py until we have something specific for all meta-estimators.

Copy link
Member

@NicolasHug NicolasHug left a comment

Still LGTM after the changes ;)

@amueller amueller merged commit 5925fb9 into scikit-learn:master Jul 29, 2019
17 checks passed
@amueller
Copy link
Member

@amueller amueller commented Jul 29, 2019

thanks! (grr I forgot to fix the merge message again, sorry!)

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

5 participants