8000 Remove support for classes in check_estimator and parametrize_with_checks · Issue #17030 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

Remove support for classes in check_estimator and parametrize_with_checks #17030

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
Closed
NicolasHug opened this issue Apr 24, 2020 · 2 comments · Fixed by #17032
Closed

Remove support for classes in check_estimator and parametrize_with_checks #17030

NicolasHug opened this issue Apr 24, 2020 · 2 comments · Fixed by #17032

Comments

@NicolasHug
Copy link
Member

We currently partially support classes on check_estimator() or parametrize_with_checks(). However, they don't work in general when the class requires some parameter with no default (except the hard-coded special cases of 'estimator' and 'base_estimator').

As far as I understand, the only thing that class support adds is the additional check_parameters_default_constructible check.

Class support leads to complicated logic for example with the xfail_checks tag where we need to check if we can first construct and instance, and if we can't, we just silently ignore the tag.

Couldn't we just let check_estimator() and parametrize_with_checks work only on instances, and publicly expose check_parameters_default_constructible separately? I think it would make the test suite logic significantly simpler. In terms of usage, it's a very mild inconvenience.

CC @rth @thomasjpfan
@rth
Copy link
Member
rth commented Apr 24, 2020

+1. We can still introduce the deprecation warning in this release.

Deprecating class shouldn't be an issue for things like #11324 right? cc @jnothman

In any case this would go in the direction that instead of default __init__ parameters, testing different combinations of init parameter is probably recommended (at least for solvers etc).

@thomasjpfan
Copy link
Member

From my understanding, check_estimator started with only classes and then instance support was added in #9019 .

I would be in favor of removing support for classes.

@NicolasHug NicolasHug mentioned this issue Apr 24, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@rth @NicolasHug @thomasjpfan
0