Skip to content

TST enable to run common test on stacking and voting estimators #18045

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Oct 8, 2020
Merged

TST enable to run common test on stacking and voting estimators #18045

merged 6 commits into from
Oct 8, 2020

Conversation

glemaitre
Copy link
Member

This PRs introduce a way to construct Stacking and Voting estimators to run the common test on them.

jnothman
jnothman reviewed Aug 1, 2020
View reviewed changes
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.

I wonder if this was too strict an assumption, although I suppose there's no case that would have previously worked that now would break?

sklearn/utils/estimator_checks.py
@@ -335,10 +337,24 @@ def _construct_instance(Estimator):
estimator = Estimator(Ridge())
else:
estimator = Estimator(LinearDiscriminantAnalysis())
elif required_parameters in (['estimators'],):
# Heterogeneous ensemble classes (i.e. stacking, voting)
if issubclass(Estimator, RegressorMixin):
Copy link
Member

Choose a reason for hiding this comment

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

except that just because the meta is a regressor, it doesn't mean it consists of regressors...

Copy link
Member Author

Choose a reason for hiding this comment

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

I would say that it could be a reasonably default for the moment (we can add a meaningful comment).
Right now we are not testing every combination in common tests even for other estimators so it might be a start?

@glemaitre
Copy link
Member Author

We need to remove the check_estimator call in the test_* file to not run twice the same tests.

@glemaitre glemaitre mentioned this pull request Aug 13, 2020
Merged
thomasjpfan
thomasjpfan approved these changes Aug 13, 2020
View reviewed changes
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

rth
rth approved these changes Aug 13, 2020
View reviewed changes
Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

LGTM for me as well. We can merge unless @jnothman has other comments/concerns.

jnothman
jnothman reviewed Aug 15, 2020
View reviewed changes
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.

I think this is a bit safer than for the *SearchCV changes... but I still believe that it may be unhelpful to putting this special-casing of instance construction into a public module, when you really want to special-case the handling within scikit-learn.

@rth
Copy link
Member

rth commented Aug 15, 2020

but I still believe that it may be unhelpful to putting this special-casing of instance construction into a public module, when you really want to special-case the handling within scikit-learn.

This change is done in a in a private function though, that,

  • is used for generating the list of instances for test_estimators which does not affect users
  • is used in check_parameters_default_constructible I guess this is more debatable but I'm not sure how else we are going to check that such meta-esimators with the estimators positional flag are default constructible. Something like [WIP] API specify test parameters via classmethod #11324 could have been better long term.

I don't have strong feelings about this (aside for the fact that we should get rid of check_estimator in tests in favor of parametrized common tests). I guess that could also be done with a separate test function(s) as previously done, but might still need some mechanism for check_parameters_default_constructible?

Also I would really like to merge #18054 though. Do you understand why that test fails there but not after this PR?

@glemaitre
Copy link
Member Author

Do you understand why that test fails there but not after this PR?

It seems that _maybe_skip does not work as expected. I should check that indeed.

@glemaitre
Copy link
Member Author

My bad, we don't need this PR for #18054, I did a silly typo but I am still thinking that this PR is relevant.

@adrinjalali
Copy link
Member

I'd re-merge master before merging into master :)

@cmarmo
Copy link
Contributor

cmarmo commented Oct 8, 2020

I'd re-merge master before merging into master :)

@adrinjalali done and it's green... shall be merged? Thanks! :)

adrinjalali
adrinjalali approved these changes Oct 8, 2020
View reviewed changes
Copy link
Member

@adrinjalali adrinjalali left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @glemaitre , and thanks @cmarmo for the reminder :)

@adrinjalali adrinjalali merged commit 2199de6 into scikit-learn:master Oct 8, 2020
amrcode pushed a commit to amrcode/scikit-learn that referenced this pull request Oct 19, 2020
@glemaitre @amrcode
TST enable to run common test on stacking and voting estimators (scik…
9f882c9 
…it-learn#18045)

* TST enable to run common test on stacking and voting estimators

* revert config changes

* revert unecessary change in _testing

* TST remove check_estimator from local files

* PEP8
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
@glemaitre @jayzed82
TST enable to run common test on stacking and voting estimators (scik…
35eaec7 
…it-learn#18045)

* TST enable to run common test on stacking and voting estimators

* revert config changes

* revert unecessary change in _testing

* TST remove check_estimator from local files

* PEP8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnothman jnothman jnothman left review comments

@agramfort agramfort agramfort approved these changes

@rth rth rth approved these changes

@adrinjalali adrinjalali adrinjalali approved these changes

@thomasjpfan thomasjpfan thomasjpfan approved these changes

Assignees
No one assigned
Labels
module:utils
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@glemaitre @rth @adrinjalali @cmarmo @jnothman @agramfort @thomasjpfan