8000 [MRG] Deprecate class support for check_estimator and parametrize_with_checks by NicolasHug · Pull Request #17032 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

[MRG] Deprecate class support for check_estimator and parametrize_with_checks #17032

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 15 commits into from
Apr 27, 2020
Merged

[MRG] Deprecate class support for check_estimator and parametrize_with_checks #17032

merged 15 commits into from
Apr 27, 2020

Conversation

NicolasHug
Copy link
Member
@NicolasHug NicolasHug commented Apr 24, 2020 •
edited
Loading

Fix #17030

This PR deprecates passing a class to check_estimator and parametrize_with_checks. A new check_class utility is created. Hopefully, this will make the code simpler and our lives easier in the long run.

CC @rth @thomasjpfan

Maybe also @adrinjalali @jnothman @amueller would want to have a look

NicolasHug
NicolasHug commented Apr 24, 2020
View reviewed changes
sklearn/tests/test_common.py
@@ -105,7 +107,7 @@ def _tested_estimators():
yield estimator


@parametrize_with_checks(_tested_estimators())
@parametrize_with_checks(list(_tested_estimators()))
Copy link
Member Author

Choose a reason for hiding this comment

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

without this, the iterator would be consumed in parametrize_with_checks due to the addition of if any(isinstance(est, type) for est in estimators):

Note that the docstring of parametrize_with_checks says that its argument should be a list.

@NicolasHug NicolasHug changed the title [WIP] Deprecate class support for check_estimator and parametrize_with_checks [MRG] Deprecate class support for check_estimator and parametrize_with_checks Apr 24, 2020
@rth
Copy link
Member
rth commented Apr 26, 2020

Thanks @NicolasHug ! Generally I think this would be a good idea. A few comments,

  • check_class is not very expressive, maybe check_estimator_class instead?
  • I would remove this in 1 version instead of 2, it should have been part of the dev API not public API. In any case this check_estimator only affects tests and I think it's a bit less critical, we could discuss it on Monday.
  • Maybe deprecate check_parameters_default_constructible in favor of check_estimator_class already now.

@rth
Copy link
Member
rth commented Apr 26, 2020

A new check_class utility is created.

BTW, is this actually necessary? We could have kept check_parameters_default_constructible as part of common tests, make it support instances as input, and just get the class object from it.

jnothman
jnothman reviewed Apr 27, 2020
View reviewed changes
sklearn/utils/estimator_checks.py Outdated
@@ -477,6 +513,27 @@ def check_estimator(Estimator, generate_only=False):
warnings.warn(str(exception), SkipTestWarning)


def check_estimator_class(Estimator):
Copy link
Member

Choose a reason for hiding this comment

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

YAGNI?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep will remove

@jnothman
Copy link
Member

Can't we just run check_parameters_default_constructible but it uses the class of the instance provided?

@NicolasHug
Copy link
Member Author

Can't we just run check_parameters_default_constructible but it uses the class of the instance provided?

Yes, I'll update asap and ping when ready ;)

@NicolasHug NicolasHug added this to the 0.23 milestone Apr 27, 2020
NicolasHug
NicolasHug commented Apr 27, 2020
View reviewed changes
@NicolasHug
Copy link
Member Author

PR ready for review @rth @jnothman @adrinjalali @thomasjpfan

  • The deprecation cycle is of only 1 version
  • check_estimator() now checks the class even when passed an instance

@NicolasHug
Copy link
Member Author

maybe @ogrisel too if you have time

adrinjalali
adrinjalali approved these changes Apr 27, 2020
View reviewed changes
Copy link
Member
@adrinjalali adrinjalali 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 I think.

sklearn/utils/estimator_checks.py
@@ -385,6 +390,10 @@ def parametrize_with_checks(estimators):
estimators : list of estimators objects or classes
Estimators to generated checks for.

.. deprecated:: 0.23
Copy link
Member

Choose a reason for hiding this comment

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

the estimator is not deprecated, but changed? I'm not sure if versionchanged works better here or not.

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 guess it should be deprecated for now, and become versionchanged in 0.24

@NicolasHug
Copy link
Member Author

Thanks for the reviews! do you want to have a last look @rth? Otherwise I'll merge tomorrow

rth
rth approved these changes Apr 27, 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, thanks @NicolasHug !

@rth rth merged commit fc00415 into scikit-learn:master Apr 27, 2020
@NicolasHug
Copy link
Member Author

Thanks!

ping #17010

adrinjalali pushed a commit that referenced this pull request Apr 30, 2020
@NicolasHug @adrinjalali
Deprecate class support for check_estimator (#17032)
97af533 
and parametrize_with_checks
gio8tisu pushed a commit to gio8tisu/scikit-learn that referenced this pull request May 15, 2020
@NicolasHug @gio8tisu
Deprecate class support for check_estimator (scikit-learn#17032)
f46bb41 
and parametrize_with_checks
viclafargue pushed a commit to viclafargue/scikit-learn that referenced this pull request Jun 26, 2020
@NicolasHug @viclafargue
Deprecate class support for check_estimator (scikit-learn#17032)
70a0b46 
and parametrize_with_checks
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

@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
0.23
Development

Successfully merging this pull request may close these issues.

Remove support for classes in check_estimator and parametrize_with_checks
5 participants
@NicolasHug @rth @jnothman @adrinjalali @thomasjpfan
0