Skip to content

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

Merged
rth merged 15 commits into
scikit-learn:masterfrom
NicolasHug:remove_class_support_check_estimators
Apr 27, 2020
Merged

[MRG] Deprecate class support for check_estimator and parametrize_with_checks#17032
rth merged 15 commits into
scikit-learn:masterfrom
NicolasHug:remove_class_support_check_estimators

Conversation

@NicolasHug

@NicolasHug NicolasHug commented Apr 24, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

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
Comment thread sklearn/tests/test_common.py


@parametrize_with_checks(_tested_estimators())
@parametrize_with_checks(list(_tested_estimators()))

@NicolasHug NicolasHug Apr 24, 2020

Copy link
Copy Markdown
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

rth commented Apr 26, 2020

Copy link
Copy Markdown
Member

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

rth commented Apr 26, 2020

Copy link
Copy Markdown
Member

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
Comment thread sklearn/utils/estimator_checks.py Outdated
warnings.warn(str(exception), SkipTestWarning)


def check_estimator_class(Estimator):

@jnothman jnothman Apr 27, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

YAGNI?

@NicolasHug NicolasHug Apr 27, 2020

Copy link
Copy Markdown
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

jnothman commented Apr 27, 2020

Copy link
Copy Markdown
Member

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

@NicolasHug

NicolasHug commented Apr 27, 2020

Copy link
Copy Markdown
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
Comment thread doc/whats_new/v0.23.rst Outdated
@NicolasHug

NicolasHug commented Apr 27, 2020

Copy link
Copy Markdown
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

NicolasHug commented Apr 27, 2020

Copy link
Copy Markdown
Member Author

maybe @ogrisel too if you have time

adrinjalali
adrinjalali approved these changes Apr 27, 2020
View reviewed changes

@adrinjalali adrinjalali left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Otherwise LGTM I think.

Comment thread sklearn/utils/estimator_checks.py
estimators : list of estimators objects or classes
Estimators to generated checks for.

.. deprecated:: 0.23

@adrinjalali adrinjalali Apr 27, 2020

Copy link
Copy Markdown
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.

@NicolasHug NicolasHug Apr 27, 2020

Copy link
Copy Markdown
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

NicolasHug commented Apr 27, 2020

Copy link
Copy Markdown
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

@rth rth left a comment

Copy link
Copy Markdown
Member

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

NicolasHug commented Apr 27, 2020

Copy link
Copy Markdown
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
@adrinjalali adrinjalali adrinjalali approved these changes
@thomasjpfan thomasjpfan thomasjpfan approved these changes
+1 more reviewer
@rth rth rth approved these changes
Reviewers whose approvals may not affect merge requirements

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