[MRG] FIX run test for meta-estimator having estimators keyword #14305
Conversation
|
I would be happy to have some feedback @amueller @jnothman @thomasjpfan @NicolasHug |
doc/whats_new/v0.22.rst
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
since this is not in the utils module section, the link is broken ;)
sklearn/utils/estimator_checks.py
Outdated
| @@ -2165,6 +2173,21 @@ def check_parameters_default_constructible(name, Estimator): | |||
| estimator = Estimator(Ridge()) | |||
| else: | |||
| estimator = Estimator(LinearDiscriminantAnalysis()) | |||
| elif "estimators" in required_parameters: | |||
There was a problem hiding this comment.
It seems that this whole estimator initialization is duplicated between check_parameters_default_constructible and _tested_estimators. Might be worth considering a unifying helper?
There was a problem hiding this comment.
I agree but I would do that in another PR or go for a better solution as proposed by @jnothman.
|
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. |
|
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? |
|
@NicolasHug I should have addressed all comments |
sklearn/utils/estimator_checks.py
Outdated
| @@ -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') | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This is necessary otherwise predict_proba is not defined and raise an error (leading to some failure in the common tests).
doc/whats_new/v0.22.rst
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
I would say "run it by default" because you could already run it by giving it an instance.
|
I'd rather explicitly run check_estimator for the relevant meta-estimators
than put this temporarily in estimator checks.
|
|
@jnothman I moved the tests in |
|
thanks! (grr I forgot to fix the merge message again, sorry!) |
We should start to run tests for meta-estimator like
VotingClassifierandVotingRegressor.This is also useful for the stackingclassifier and stackingregressor for which we should ensure to pass these tests.