From d53ec50d5c8f24dabce06f2198d3e4e1573ccb49 Mon Sep 17 00:00:00 2001 From: Nicolas Hug Date: Fri, 24 Apr 2020 12:58:38 -0400 Subject: [PATCH 01/12] WIP --- sklearn/utils/estimator_checks.py | 11 ++++++++++- sklearn/utils/tests/test_estimator_checks.py | 5 +++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/sklearn/utils/estimator_checks.py b/sklearn/utils/estimator_checks.py index efac2aca2a2df..c9607e6cf9388 100644 --- a/sklearn/utils/estimator_checks.py +++ b/sklearn/utils/estimator_checks.py @@ -439,9 +439,14 @@ def check_estimator(Estimator, generate_only=False): Parameters ---------- - estimator : estimator object or class + estimator : estimator object Estimator to check. Estimator is a class object or instance. + .. versionchanged:: 0.23 + Classes are deprecated from version 0.23. Pass an instance + instead, and call `check_estimator_class` separately to check the + class. + generate_only : bool, optional (default=False) When `False`, checks are evaluated when `check_estimator` is called. When `True`, `check_estimator` returns a generator that yields @@ -458,6 +463,10 @@ def check_estimator(Estimator, generate_only=False): """ if isinstance(Estimator, type): # got a class + msg = ("Passing a class is deprecated since version 0.23. " + "Please pass an instance instead and call " + "check_estimator_class separately on the class.") + warnings.warn(msg, FutureWarning) checks_generator = _generate_class_checks(Estimator) else: # got an instance diff --git a/sklearn/utils/tests/test_estimator_checks.py b/sklearn/utils/tests/test_estimator_checks.py index a755daa842ef5..faa30c077def1 100644 --- a/sklearn/utils/tests/test_estimator_checks.py +++ b/sklearn/utils/tests/test_estimator_checks.py @@ -623,6 +623,11 @@ def test_all_estimators_all_public(): assert not est.__class__.__name__.startswith("_") +def test_check_estimator_class_warning(): + # message should be Passing a class is deprecated... + assert_warns(FutureWarning, check_estimator, LogisticRegression) + + if __name__ == '__main__': # This module is run as a script to check that we have no dependency on # pytest for estimator checks. From ccf996366598db3e4aab907f285d81c1ff00a6b7 Mon Sep 17 00:00:00 2001 From: Nicolas Hug Date: Fri, 24 Apr 2020 13:33:22 -0400 Subject: [PATCH 02/12] WIP --- sklearn/tests/test_common.py | 16 ++++++++++++++++ sklearn/utils/estimator_checks.py | 14 +++++++++++++- sklearn/utils/tests/test_estimator_checks.py | 8 +++----- 3 files changed, 32 insertions(+), 6 deletions(-) diff --git a/sklearn/tests/test_common.py b/sklearn/tests/test_common.py index af98c1bc50a74..71f67c46f78d8 100644 --- a/sklearn/tests/test_common.py +++ b/sklearn/tests/test_common.py @@ -48,7 +48,9 @@ def test_all_estimator_no_base_class(): assert not name.lower().startswith('base'), msg +@ignore_warnings("Passing a class is depr", category=FutureWarning) # 0.25 def test_estimator_cls_parameterize_with_checks(): + # TODO: remove test in 0.25 # Non-regression test for #16707 to ensure that parametrize_with_checks # works with estimator classes param_checks = parametrize_with_checks([LogisticRegression]) @@ -115,7 +117,9 @@ def test_estimators(estimator, check, request): check(estimator) +@ignore_warnings("Passing a class is depr", category=FutureWarning) # 0.25 def test_check_estimator_generate_only(): + # TODO in 0.25: remove checks on passing a class estimator_cls_gen_checks = check_estimator(LogisticRegression, generate_only=True) all_instance_gen_checks = check_estimator(LogisticRegression(), @@ -238,3 +242,15 @@ def test_all_tests_are_importable(): '__init__.py or an add_subpackage directive ' 'in the parent ' 'setup.py'.format(missing_tests)) + + +def test_class_deprecated(): + # Make sure passing classes to check_estimator or parametrize_with_checks + # is deprecated + + msg = "Passing a class is deprecated" + with pytest.warns(FutureWarning, match=msg): + check_estimator(LogisticRegression) + + with pytest.warns(FutureWarning, match=msg): + parametrize_with_checks([LogisticRegression]) diff --git a/sklearn/utils/estimator_checks.py b/sklearn/utils/estimator_checks.py index c9607e6cf9388..aec4a577bb5e8 100644 --- a/sklearn/utils/estimator_checks.py +++ b/sklearn/utils/estimator_checks.py @@ -351,6 +351,7 @@ def _mark_xfail_checks(estimator, check, pytest): """Mark (estimator, check) pairs with xfail according to the _xfail_checks_ tag""" if isinstance(estimator, type): + # TODO: remove in 0.25 since passing instances isn't supported anymore # try to construct estimator instance, if it is unable to then # return the estimator class, ignoring the tag try: @@ -402,6 +403,14 @@ def parametrize_with_checks(estimators): """ import pytest + if any(isinstance(est, type) for est in estimators): + # TODO: remove class support in 0.25 + msg = ("Passing a class is deprecated since version 0.23 " + "and won't be supported in 0.25." + "Please pass an instance instead and call " + "check_estimator_class separately on the class.") + warnings.warn(msg, FutureWarning) + checks_generator = chain.from_iterable( check_estimator(estimator, generate_only=True) for estimator in estimators) @@ -461,12 +470,15 @@ def check_estimator(Estimator, generate_only=False): Generator that yields (estimator, check) tuples. Returned when `generate_only=True`. """ + # TODO: remove class support in 0.25 if isinstance(Estimator, type): # got a class - msg = ("Passing a class is deprecated since version 0.23. " + msg = ("Passing a class is deprecated since version 0.23 " + "and won't be supported in 0.25." "Please pass an instance instead and call " "check_estimator_class separately on the class.") warnings.warn(msg, FutureWarning) + checks_generator = _generate_class_checks(Estimator) else: # got an instance diff --git a/sklearn/utils/tests/test_estimator_checks.py b/sklearn/utils/tests/test_estimator_checks.py index faa30c077def1..a621ede38499a 100644 --- a/sklearn/utils/tests/test_estimator_checks.py +++ b/sklearn/utils/tests/test_estimator_checks.py @@ -356,6 +356,7 @@ def fit(self, X, y): check_fit_score_takes_y("test", TestEstimatorWithDeprecatedFitMethod()) +@ignore_warnings("Passing a class is depr", category=FutureWarning) # 0.25 def test_check_estimator(): # tests that the estimator actually fails on "bad" estimators. # not a complete test of all checks, which are very extensive. @@ -579,7 +580,9 @@ def test_check_regressor_data_not_an_array(): EstimatorInconsistentForPandas()) +@ignore_warnings("Passing a class is depr", category=FutureWarning) # 0.25 def test_check_estimator_required_parameters_skip(): + # TODO: remove whole test in 0.25 class MyEstimator(BaseEstimator): _required_parameters = ["special_parameter"] @@ -623,11 +626,6 @@ def test_all_estimators_all_public(): assert not est.__class__.__name__.startswith("_") -def test_check_estimator_class_warning(): - # message should be Passing a class is deprecated... - assert_warns(FutureWarning, check_estimator, LogisticRegression) - - if __name__ == '__main__': # This module is run as a script to check that we have no dependency on # pytest for estimator checks. From c013f4f2f8404300ee7cd15e950825ed2e770bc2 Mon Sep 17 00:00:00 2001 From: Nicolas Hug Date: Fri, 24 Apr 2020 14:20:12 -0400 Subject: [PATCH 03/12] fix test --- sklearn/utils/estimator_checks.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sklearn/utils/estimator_checks.py b/sklearn/utils/estimator_checks.py index aec4a577bb5e8..0cb13ba0bb19d 100644 --- a/sklearn/utils/estimator_checks.py +++ b/sklearn/utils/estimator_checks.py @@ -396,7 +396,8 @@ def parametrize_with_checks(estimators): >>> from sklearn.linear_model import LogisticRegression >>> from sklearn.tree import DecisionTreeRegressor - >>> @parametrize_with_checks([LogisticRegression, DecisionTreeRegressor]) + >>> @parametrize_with_checks([LogisticRegression(), + ... DecisionTreeRegressor()]) ... def test_sklearn_compatible_estimator(estimator, check): ... check(estimator) From ddd67db61ceee0aef56ee20c1a27d1eb47aeb64d Mon Sep 17 00:00:00 2001 From: Nicolas Hug Date: Fri, 24 Apr 2020 15:01:07 -0400 Subject: [PATCH 04/12] avoid consume generator --- sklearn/tests/test_common.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sklearn/tests/test_common.py b/sklearn/tests/test_common.py index 71f67c46f78d8..dd90a1c6bb876 100644 --- a/sklearn/tests/test_common.py +++ b/sklearn/tests/test_common.py @@ -107,7 +107,7 @@ def _tested_estimators(): yield estimator -@parametrize_with_checks(_tested_estimators()) +@parametrize_with_checks(list(_tested_estimators())) def test_estimators(estimator, check, request): # Common tests for estimator instances with ignore_warnings(category=(FutureWarning, From a4bca6ca6f06cc5ae84a3d5886ce52e0942af70e Mon Sep 17 00:00:00 2001 From: Nicolas Hug Date: Fri, 24 Apr 2020 15:28:17 -0400 Subject: [PATCH 05/12] some more --- doc/developers/develop.rst | 13 ++++--- doc/modules/classes.rst | 1 + sklearn/tests/test_common.py | 11 ++++-- sklearn/utils/estimator_checks.py | 58 +++++++++++++++++++++++++------ 4 files changed, 65 insertions(+), 18 deletions(-) diff --git a/doc/developers/develop.rst b/doc/developers/develop.rst index f17c58cee0d7f..7164b0c375cb1 100644 --- a/doc/developers/develop.rst +++ b/doc/developers/develop.rst @@ -246,13 +246,16 @@ whether it is just for you or for contributing it to scikit-learn, there are several internals of scikit-learn that you should be aware of in addition to the scikit-learn API outlined above. You can check whether your estimator adheres to the scikit-learn interface and standards by running -:func:`utils.estimator_checks.check_estimator` on the class or using -:func:`~sklearn.utils.parametrize_with_checks` pytest decorator (see its -docstring for details and possible interactions with `pytest`):: +:func:`~sklearn.utils.estimator_checks.check_estimator` on an instance and +:func:`~sklearn.utils.check_estimator.check_class` on the class. The +:func:`~sklearn.utils.parametrize_with_checks` pytest decorator can also be +used to generate the instance checks (see its docstring for details and +possible interactions with `pytest`):: - >>> from sklearn.utils.estimator_checks import check_estimator + >>> from sklearn.utils.estimator_checks import check_estimator, check_class >>> from sklearn.svm import LinearSVC - >>> check_estimator(LinearSVC) # passes + >>> check_estimator(LinearSVC()) # passes + >>> check_class(LinearSVC) # passes The main motivation to make a class compatible to the scikit-learn estimator interface might be that you want to use it together with model evaluation and diff --git a/doc/modules/classes.rst b/doc/modules/classes.rst index 3d9924638b69b..6e30c617c7e99 100644 --- a/doc/modules/classes.rst +++ b/doc/modules/classes.rst @@ -1568,6 +1568,7 @@ Plotting utils.class_weight.compute_sample_weight utils.deprecated utils.estimator_checks.check_estimator + utils.estimator_checks.check_class utils.estimator_checks.parametrize_with_checks utils.extmath.safe_sparse_dot utils.extmath.randomized_range_finder diff --git a/sklearn/tests/test_common.py b/sklearn/tests/test_common.py index dd90a1c6bb876..007932eb3cf2c 100644 --- a/sklearn/tests/test_common.py +++ b/sklearn/tests/test_common.py @@ -20,7 +20,7 @@ from sklearn.utils import all_estimators from sklearn.utils._testing import ignore_warnings from sklearn.exceptions import ConvergenceWarning -from sklearn.utils.estimator_checks import check_estimator +from sklearn.utils.estimator_checks import check_estimator, check_class import sklearn from sklearn.base import BiclusterMixin @@ -244,7 +244,7 @@ def test_all_tests_are_importable(): 'setup.py'.format(missing_tests)) -def test_class_deprecated(): +def test_class_support_deprecated(): # Make sure passing classes to check_estimator or parametrize_with_checks # is deprecated @@ -254,3 +254,10 @@ def test_class_deprecated(): with pytest.warns(FutureWarning, match=msg): parametrize_with_checks([LogisticRegression]) + + +def test_check_classs(): + + check_class(LogisticRegression) + with pytest.raises(ValueError, match="expects a Class object"): + check_class(LogisticRegression()) diff --git a/sklearn/utils/estimator_checks.py b/sklearn/utils/estimator_checks.py index 0cb13ba0bb19d..ed27fb74aacd8 100644 --- a/sklearn/utils/estimator_checks.py +++ b/sklearn/utils/estimator_checks.py @@ -33,7 +33,7 @@ from ..linear_model import Ridge from ..base import (clone, ClusterMixin, is_classifier, is_regressor, - RegressorMixin, is_outlier_detector) + RegressorMixin, is_outlier_detector, BaseEstimator) from ..metrics import accuracy_score, adjusted_rand_score, f1_score from ..random_projection import BaseRandomProjection @@ -333,12 +333,15 @@ def _construct_instance(Estimator): return estimator +# TODO: probably not needed anymore in 0.25 since _generate_class_checks should +# be removed too. Just put this in check_estimator() def _generate_instance_checks(name, estimator): """Generate instance checks.""" yield from ((estimator, partial(check, name)) for check in _yield_all_checks(name, estimator)) +# TODO: remove this in 0.25 def _generate_class_checks(Estimator): """Generate class checks.""" name = Estimator.__name__ @@ -351,9 +354,10 @@ def _mark_xfail_checks(estimator, check, pytest): """Mark (estimator, check) pairs with xfail according to the _xfail_checks_ tag""" if isinstance(estimator, type): - # TODO: remove in 0.25 since passing instances isn't supported anymore # try to construct estimator instance, if it is unable to then # return the estimator class, ignoring the tag + # TODO: remove this if block in 0.25 since passing instances isn't + # supported anymore try: estimator = _construct_instance(estimator) except Exception: @@ -386,6 +390,11 @@ def parametrize_with_checks(estimators): estimators : list of estimators objects or classes Estimators to generated checks for. + .. versionchanged:: 0.23 + Passing a class is deprecated from version 0.23, and won't be + supported in 0.25. Pass an instance instead, and call + `check_class` separately to check the class. + Returns ------- decorator : `pytest.mark.parametrize` @@ -405,11 +414,12 @@ def parametrize_with_checks(estimators): import pytest if any(isinstance(est, type) for est in estimators): - # TODO: remove class support in 0.25 + # TODO: remove class support in 0.25 and update docstrings telling + # users to use check_class as well msg = ("Passing a class is deprecated since version 0.23 " "and won't be supported in 0.25." "Please pass an instance instead and call " - "check_estimator_class separately on the class.") + "check_class separately on the class.") warnings.warn(msg, FutureWarning) checks_generator = chain.from_iterable( @@ -428,7 +438,7 @@ def check_estimator(Estimator, generate_only=False): """Check if estimator adheres to scikit-learn conventions. This estimator will run an extensive test-suite for input validation, - shapes, etc, making sure that the estimator complies with `scikit-leanrn` + shapes, etc, making sure that the estimator complies with `scikit-learn` conventions as detailed in :ref:`rolling_your_own_estimator`. Additional tests for classifiers, regressors, clustering or transformers will be run if the Estimator class inherits from the corresponding mixin @@ -436,7 +446,10 @@ def check_estimator(Estimator, generate_only=False): This test can be applied to classes or instances. Classes currently have some additional tests that related to construction, - while passing instances allows the testing of multiple options. + while passing instances allows the testing of multiple options. However, + support for classes is deprecated since version 0.23 and will be removed + in version 0.25. Class checks can instead be run using + `~sklearn.utils.estimator_checks.check_class`. Setting `generate_only=True` returns a generator that yields (estimator, check) tuples where the check can be called independently from each @@ -453,9 +466,9 @@ def check_estimator(Estimator, generate_only=False): Estimator to check. Estimator is a class object or instance. .. versionchanged:: 0.23 - Classes are deprecated from version 0.23. Pass an instance - instead, and call `check_estimator_class` separately to check the - class. + Passing a class is deprecated from version 0.23, and won't be + supported in 0.25. Pass an instance instead, and call + `check_class` separately to check the class. generate_only : bool, optional (default=False) When `False`, checks are evaluated when `check_estimator` is called. @@ -471,13 +484,14 @@ def check_estimator(Estimator, generate_only=False): Generator that yields (estimator, check) tuples. Returned when `generate_only=True`. """ - # TODO: remove class support in 0.25 + # TODO: remove class support in 0.25 and update docstrings telling users to + # use check_class as well if isinstance(Estimator, type): # got a class msg = ("Passing a class is deprecated since version 0.23 " "and won't be supported in 0.25." "Please pass an instance instead and call " - "check_estimator_class separately on the class.") + "check_class separately on the class.") warnings.warn(msg, FutureWarning) checks_generator = _generate_class_checks(Estimator) @@ -499,6 +513,26 @@ def check_estimator(Estimator, generate_only=False): warnings.warn(str(exception), SkipTestWarning) +def check_class(Estimator): + """Check if an estimator class adheres to scikit-learn conventions. + + This check suite should be used along with + `~sklearn.utils.estimator_checks.check_estimator` to also check the + estimator instance. Using `check_class` alone or `check_estimator` alone + isn't sufficient for full compatibility. + + Parameters + ---------- + Estimator : class + Estimator class to check. + """ + if isinstance(Estimator, BaseEstimator): + raise ValueError("check_class() expects a Class object to be passed. " + "It looks like you passed an estimator instance. " + "You should use check_estimator for instances.") + check_parameters_default_constructible(Estimator.__name__, Estimator) + + def _boston_subset(n_samples=200): global BOSTON if BOSTON is None: @@ -2588,6 +2622,8 @@ def check_estimators_data_not_an_array(name, estimator_orig, X, y, obj_type): assert_allclose(pred1, pred2, atol=1e-2, err_msg=name) +# TODO: This check can probably be removed in 0.25 since _generate_class_checks +# should be removed too. Maybe put this inside of `check_class` instead. def check_parameters_default_constructible(name, Estimator): # this check works on classes, not instances # test default-constructibility From e63e97cb88471fe880c87957244ca940c3603e95 Mon Sep 17 00:00:00 2001 From: Nicolas Hug Date: Fri, 24 Apr 2020 15:30:35 -0400 Subject: [PATCH 06/12] name --- sklearn/tests/test_common.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sklearn/tests/test_common.py b/sklearn/tests/test_common.py index 007932eb3cf2c..957b2eb3f9ded 100644 --- a/sklearn/tests/test_common.py +++ b/sklearn/tests/test_common.py @@ -256,8 +256,7 @@ def test_class_support_deprecated(): parametrize_with_checks([LogisticRegression]) -def test_check_classs(): - +def test_check_class(): check_class(LogisticRegression) with pytest.raises(ValueError, match="expects a Class object"): check_class(LogisticRegression()) From 6b36364b749382f390e312e9217fd4f1b1a9fbc8 Mon Sep 17 00:00:00 2001 From: Nicolas Hug Date: Fri, 24 Apr 2020 15:33:58 -0400 Subject: [PATCH 07/12] comments --- sklearn/utils/estimator_checks.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/sklearn/utils/estimator_checks.py b/sklearn/utils/estimator_checks.py index ed27fb74aacd8..623d7e3abf9b6 100644 --- a/sklearn/utils/estimator_checks.py +++ b/sklearn/utils/estimator_checks.py @@ -390,7 +390,7 @@ def parametrize_with_checks(estimators): estimators : list of estimators objects or classes Estimators to generated checks for. - .. versionchanged:: 0.23 + .. deprecated:: 0.23 Passing a class is deprecated from version 0.23, and won't be supported in 0.25. Pass an instance instead, and call `check_class` separately to check the class. @@ -465,7 +465,7 @@ def check_estimator(Estimator, generate_only=False): estimator : estimator object Estimator to check. Estimator is a class object or instance. - .. versionchanged:: 0.23 + .. deprecated:: 0.23 Passing a class is deprecated from version 0.23, and won't be supported in 0.25. Pass an instance instead, and call `check_class` separately to check the class. @@ -2623,7 +2623,8 @@ def check_estimators_data_not_an_array(name, estimator_orig, X, y, obj_type): # TODO: This check can probably be removed in 0.25 since _generate_class_checks -# should be removed too. Maybe put this inside of `check_class` instead. +# should be removed too, and this is the only class-specific check. Maybe this +# should just become the body of check_class() instead. def check_parameters_default_constructible(name, Estimator): # this check works on classes, not instances # test default-constructibility From fe2f4b6a6650753554c33ca22d32bb497622cd05 Mon Sep 17 00:00:00 2001 From: Nicolas Hug Date: Sun, 26 Apr 2020 12:19:32 -0400 Subject: [PATCH 08/12] renamed to check_estimator_class --- doc/developers/develop.rst | 7 ++++--- doc/modules/classes.rst | 2 +- sklearn/tests/test_common.py | 9 +++++---- sklearn/utils/estimator_checks.py | 7 ++++--- 4 files changed, 14 insertions(+), 11 deletions(-) diff --git a/doc/developers/develop.rst b/doc/developers/develop.rst index 7164b0c375cb1..77e411d9a23af 100644 --- a/doc/developers/develop.rst +++ b/doc/developers/develop.rst @@ -247,15 +247,16 @@ several internals of scikit-learn that you should be aware of in addition to the scikit-learn API outlined above. You can check whether your estimator adheres to the scikit-learn interface and standards by running :func:`~sklearn.utils.estimator_checks.check_estimator` on an instance and -:func:`~sklearn.utils.check_estimator.check_class` on the class. The +:func:`~sklearn.utils.check_estimator.check_estimator_class` on the class. The :func:`~sklearn.utils.parametrize_with_checks` pytest decorator can also be used to generate the instance checks (see its docstring for details and possible interactions with `pytest`):: - >>> from sklearn.utils.estimator_checks import check_estimator, check_class + >>> from sklearn.utils.estimator_checks import check_estimator + >>> from sklearn.utils.estimator_checks import check_estimator_class >>> from sklearn.svm import LinearSVC >>> check_estimator(LinearSVC()) # passes - >>> check_class(LinearSVC) # passes + >>> check_estimator_class(LinearSVC) # passes The main motivation to make a class compatible to the scikit-learn estimator interface might be that you want to use it together with model evaluation and diff --git a/doc/modules/classes.rst b/doc/modules/classes.rst index 6e30c617c7e99..333a641dfc1a9 100644 --- a/doc/modules/classes.rst +++ b/doc/modules/classes.rst @@ -1568,7 +1568,7 @@ Plotting utils.class_weight.compute_sample_weight utils.deprecated utils.estimator_checks.check_estimator - utils.estimator_checks.check_class + utils.estimator_checks.check_estimator_class utils.estimator_checks.parametrize_with_checks utils.extmath.safe_sparse_dot utils.extmath.randomized_range_finder diff --git a/sklearn/tests/test_common.py b/sklearn/tests/test_common.py index 957b2eb3f9ded..acd357f965120 100644 --- a/sklearn/tests/test_common.py +++ b/sklearn/tests/test_common.py @@ -20,7 +20,8 @@ from sklearn.utils import all_estimators from sklearn.utils._testing import ignore_warnings from sklearn.exceptions import ConvergenceWarning -from sklearn.utils.estimator_checks import check_estimator, check_class +from sklearn.utils.estimator_checks import check_estimator +from sklearn.utils.estimator_checks import check_estimator_class import sklearn from sklearn.base import BiclusterMixin @@ -256,7 +257,7 @@ def test_class_support_deprecated(): parametrize_with_checks([LogisticRegression]) -def test_check_class(): - check_class(LogisticRegression) +def test_check_estimator_class(): + check_estimator_class(LogisticRegression) with pytest.raises(ValueError, match="expects a Class object"): - check_class(LogisticRegression()) + check_estimator_class(LogisticRegression()) diff --git a/sklearn/utils/estimator_checks.py b/sklearn/utils/estimator_checks.py index 623d7e3abf9b6..7b21d2dfb11a4 100644 --- a/sklearn/utils/estimator_checks.py +++ b/sklearn/utils/estimator_checks.py @@ -513,7 +513,7 @@ def check_estimator(Estimator, generate_only=False): warnings.warn(str(exception), SkipTestWarning) -def check_class(Estimator): +def check_estimator_class(Estimator): """Check if an estimator class adheres to scikit-learn conventions. This check suite should be used along with @@ -527,7 +527,8 @@ def check_class(Estimator): Estimator class to check. """ if isinstance(Estimator, BaseEstimator): - raise ValueError("check_class() expects a Class object to be passed. " + raise ValueError("check_estimator_class() expects a Class object " + "to be passed. " "It looks like you passed an estimator instance. " "You should use check_estimator for instances.") check_parameters_default_constructible(Estimator.__name__, Estimator) @@ -2624,7 +2625,7 @@ def check_estimators_data_not_an_array(name, estimator_orig, X, y, obj_type): # TODO: This check can probably be removed in 0.25 since _generate_class_checks # should be removed too, and this is the only class-specific check. Maybe this -# should just become the body of check_class() instead. +# should just become the body of check_estimator_class() instead. def check_parameters_default_constructible(name, Estimator): # this check works on classes, not instances # test default-constructibility From b307f2da99dbc21d472ae7d1bbd5a3fbb23d2102 Mon Sep 17 00:00:00 2001 From: Nicolas Hug Date: Mon, 27 Apr 2020 10:17:04 -0400 Subject: [PATCH 09/12] addressed Romans comments --- doc/developers/develop.rst | 8 ++--- doc/modules/classes.rst | 1 - sklearn/tests/test_common.py | 9 ++--- sklearn/utils/estimator_checks.py | 55 +++++++++---------------------- 4 files changed, 20 insertions(+), 53 deletions(-) diff --git a/doc/developers/develop.rst b/doc/developers/develop.rst index 77e411d9a23af..13d2010ca7319 100644 --- a/doc/developers/develop.rst +++ b/doc/developers/develop.rst @@ -246,17 +246,13 @@ whether it is just for you or for contributing it to scikit-learn, there are several internals of scikit-learn that you should be aware of in addition to the scikit-learn API outlined above. You can check whether your estimator adheres to the scikit-learn interface and standards by running -:func:`~sklearn.utils.estimator_checks.check_estimator` on an instance and -:func:`~sklearn.utils.check_estimator.check_estimator_class` on the class. The +:func:`~sklearn.utils.estimator_checks.check_estimator` on an instance. The :func:`~sklearn.utils.parametrize_with_checks` pytest decorator can also be -used to generate the instance checks (see its docstring for details and -possible interactions with `pytest`):: +used (see its docstring for details and possible interactions with `pytest`):: >>> from sklearn.utils.estimator_checks import check_estimator - >>> from sklearn.utils.estimator_checks import check_estimator_class >>> from sklearn.svm import LinearSVC >>> check_estimator(LinearSVC()) # passes - >>> check_estimator_class(LinearSVC) # passes The main motivation to make a class compatible to the scikit-learn estimator interface might be that you want to use it together with model evaluation and diff --git a/doc/modules/classes.rst b/doc/modules/classes.rst index 333a641dfc1a9..3d9924638b69b 100644 --- a/doc/modules/classes.rst +++ b/doc/modules/classes.rst @@ -1568,7 +1568,6 @@ Plotting utils.class_weight.compute_sample_weight utils.deprecated utils.estimator_checks.check_estimator - utils.estimator_checks.check_estimator_class utils.estimator_checks.parametrize_with_checks utils.extmath.safe_sparse_dot utils.extmath.randomized_range_finder diff --git a/sklearn/tests/test_common.py b/sklearn/tests/test_common.py index acd357f965120..5641b2882efad 100644 --- a/sklearn/tests/test_common.py +++ b/sklearn/tests/test_common.py @@ -21,7 +21,6 @@ from sklearn.utils._testing import ignore_warnings from sklearn.exceptions import ConvergenceWarning from sklearn.utils.estimator_checks import check_estimator -from sklearn.utils.estimator_checks import check_estimator_class import sklearn from sklearn.base import BiclusterMixin @@ -245,6 +244,7 @@ def test_all_tests_are_importable(): 'setup.py'.format(missing_tests)) +# TODO: remove in 0.25 def test_class_support_deprecated(): # Make sure passing classes to check_estimator or parametrize_with_checks # is deprecated @@ -256,8 +256,5 @@ def test_class_support_deprecated(): with pytest.warns(FutureWarning, match=msg): parametrize_with_checks([LogisticRegression]) - -def test_check_estimator_class(): - check_estimator_class(LogisticRegression) - with pytest.raises(ValueError, match="expects a Class object"): - check_estimator_class(LogisticRegression()) + # Make sure check_parameters_default_constructible accepts instances now + check_parameters_default_constructible('name', LogisticRegression()) diff --git a/sklearn/utils/estimator_checks.py b/sklearn/utils/estimator_checks.py index 7b21d2dfb11a4..cbf20b522963f 100644 --- a/sklearn/utils/estimator_checks.py +++ b/sklearn/utils/estimator_checks.py @@ -392,8 +392,7 @@ def parametrize_with_checks(estimators): .. deprecated:: 0.23 Passing a class is deprecated from version 0.23, and won't be - supported in 0.25. Pass an instance instead, and call - `check_class` separately to check the class. + supported in 0.24. Pass an instance instead. Returns ------- @@ -414,12 +413,10 @@ def parametrize_with_checks(estimators): import pytest if any(isinstance(est, type) for est in estimators): - # TODO: remove class support in 0.25 and update docstrings telling - # users to use check_class as well + # TODO: remove class support in 0.24 and update docstrings msg = ("Passing a class is deprecated since version 0.23 " - "and won't be supported in 0.25." - "Please pass an instance instead and call " - "check_class separately on the class.") + "and won't be supported in 0.24." + "Please pass an instance instead.") warnings.warn(msg, FutureWarning) checks_generator = chain.from_iterable( @@ -448,8 +445,7 @@ def check_estimator(Estimator, generate_only=False): Classes currently have some additional tests that related to construction, while passing instances allows the testing of multiple options. However, support for classes is deprecated since version 0.23 and will be removed - in version 0.25. Class checks can instead be run using - `~sklearn.utils.estimator_checks.check_class`. + in version 0.24 (classe checks will still be run on the instances). Setting `generate_only=True` returns a generator that yields (estimator, check) tuples where the check can be called independently from each @@ -467,8 +463,7 @@ def check_estimator(Estimator, generate_only=False): .. deprecated:: 0.23 Passing a class is deprecated from version 0.23, and won't be - supported in 0.25. Pass an instance instead, and call - `check_class` separately to check the class. + supported in 0.24. Pass an instance instead. generate_only : bool, optional (default=False) When `False`, checks are evaluated when `check_estimator` is called. @@ -484,14 +479,12 @@ def check_estimator(Estimator, generate_only=False): Generator that yields (estimator, check) tuples. Returned when `generate_only=True`. """ - # TODO: remove class support in 0.25 and update docstrings telling users to - # use check_class as well + # TODO: remove class support in 0.24 and update docstrings if isinstance(Estimator, type): # got a class msg = ("Passing a class is deprecated since version 0.23 " - "and won't be supported in 0.25." - "Please pass an instance instead and call " - "check_class separately on the class.") + "and won't be supported in 0.24." + "Please pass an instance instead.") warnings.warn(msg, FutureWarning) checks_generator = _generate_class_checks(Estimator) @@ -513,27 +506,6 @@ def check_estimator(Estimator, generate_only=False): warnings.warn(str(exception), SkipTestWarning) -def check_estimator_class(Estimator): - """Check if an estimator class adheres to scikit-learn conventions. - - This check suite should be used along with - `~sklearn.utils.estimator_checks.check_estimator` to also check the - estimator instance. Using `check_class` alone or `check_estimator` alone - isn't sufficient for full compatibility. - - Parameters - ---------- - Estimator : class - Estimator class to check. - """ - if isinstance(Estimator, BaseEstimator): - raise ValueError("check_estimator_class() expects a Class object " - "to be passed. " - "It looks like you passed an estimator instance. " - "You should use check_estimator for instances.") - check_parameters_default_constructible(Estimator.__name__, Estimator) - - def _boston_subset(n_samples=200): global BOSTON if BOSTON is None: @@ -2623,13 +2595,16 @@ def check_estimators_data_not_an_array(name, estimator_orig, X, y, obj_type): assert_allclose(pred1, pred2, atol=1e-2, err_msg=name) -# TODO: This check can probably be removed in 0.25 since _generate_class_checks -# should be removed too, and this is the only class-specific check. Maybe this -# should just become the body of check_estimator_class() instead. def check_parameters_default_constructible(name, Estimator): # this check works on classes, not instances # test default-constructibility # get rid of deprecation warnings + if isinstance(Estimator, BaseEstimator): + # Convert estimator instance to its class + # TODO: Always convert to class in 0.24, because check_estimator() will + # only accept instances, not classes + Estimator = Estimator.__class__ + with ignore_warnings(category=FutureWarning): estimator = _construct_instance(Estimator) # test cloning From dcc41a9088dd05e1ad1bc1c7ea4d9177f391ed0f Mon Sep 17 00:00:00 2001 From: Nicolas Hug Date: Mon, 27 Apr 2020 10:21:38 -0400 Subject: [PATCH 10/12] updated deprecation version --- sklearn/tests/test_common.py | 10 +++++----- sklearn/utils/estimator_checks.py | 8 ++++---- sklearn/utils/tests/test_estimator_checks.py | 7 ++++--- 3 files changed, 13 insertions(+), 12 deletions(-) diff --git a/sklearn/tests/test_common.py b/sklearn/tests/test_common.py index 5641b2882efad..73c99b0483de8 100644 --- a/sklearn/tests/test_common.py +++ b/sklearn/tests/test_common.py @@ -48,9 +48,9 @@ def test_all_estimator_no_base_class(): assert not name.lower().startswith('base'), msg -@ignore_warnings("Passing a class is depr", category=FutureWarning) # 0.25 +@ignore_warnings("Passing a class is depr", category=FutureWarning) # 0.24 def test_estimator_cls_parameterize_with_checks(): - # TODO: remove test in 0.25 + # TODO: remove test in 0.24 # Non-regression test for #16707 to ensure that parametrize_with_checks # works with estimator classes param_checks = parametrize_with_checks([LogisticRegression]) @@ -117,9 +117,9 @@ def test_estimators(estimator, check, request): check(estimator) -@ignore_warnings("Passing a class is depr", category=FutureWarning) # 0.25 +@ignore_warnings("Passing a class is depr", category=FutureWarning) # 0.24 def test_check_estimator_generate_only(): - # TODO in 0.25: remove checks on passing a class + # TODO in 0.24: remove checks on passing a class estimator_cls_gen_checks = check_estimator(LogisticRegression, generate_only=True) all_instance_gen_checks = check_estimator(LogisticRegression(), @@ -244,7 +244,7 @@ def test_all_tests_are_importable(): 'setup.py'.format(missing_tests)) -# TODO: remove in 0.25 +# TODO: remove in 0.24 def test_class_support_deprecated(): # Make sure passing classes to check_estimator or parametrize_with_checks # is deprecated diff --git a/sklearn/utils/estimator_checks.py b/sklearn/utils/estimator_checks.py index cbf20b522963f..ec28cb22919f0 100644 --- a/sklearn/utils/estimator_checks.py +++ b/sklearn/utils/estimator_checks.py @@ -333,7 +333,7 @@ def _construct_instance(Estimator): return estimator -# TODO: probably not needed anymore in 0.25 since _generate_class_checks should +# TODO: probably not needed anymore in 0.24 since _generate_class_checks should # be removed too. Just put this in check_estimator() def _generate_instance_checks(name, estimator): """Generate instance checks.""" @@ -341,7 +341,7 @@ def _generate_instance_checks(name, estimator): for check in _yield_all_checks(name, estimator)) -# TODO: remove this in 0.25 +# TODO: remove this in 0.24 def _generate_class_checks(Estimator): """Generate class checks.""" name = Estimator.__name__ @@ -356,7 +356,7 @@ def _mark_xfail_checks(estimator, check, pytest): if isinstance(estimator, type): # try to construct estimator instance, if it is unable to then # return the estimator class, ignoring the tag - # TODO: remove this if block in 0.25 since passing instances isn't + # TODO: remove this if block in 0.24 since passing instances isn't # supported anymore try: estimator = _construct_instance(estimator) @@ -445,7 +445,7 @@ def check_estimator(Estimator, generate_only=False): Classes currently have some additional tests that related to construction, while passing instances allows the testing of multiple options. However, support for classes is deprecated since version 0.23 and will be removed - in version 0.24 (classe checks will still be run on the instances). + in version 0.24 (class checks will still be run on the instances). Setting `generate_only=True` returns a generator that yields (estimator, check) tuples where the check can be called independently from each diff --git a/sklearn/utils/tests/test_estimator_checks.py b/sklearn/utils/tests/test_estimator_checks.py index a621ede38499a..594ff65f9e889 100644 --- a/sklearn/utils/tests/test_estimator_checks.py +++ b/sklearn/utils/tests/test_estimator_checks.py @@ -356,7 +356,7 @@ def fit(self, X, y): check_fit_score_takes_y("test", TestEstimatorWithDeprecatedFitMethod()) -@ignore_warnings("Passing a class is depr", category=FutureWarning) # 0.25 +@ignore_warnings("Passing a class is depr", category=FutureWarning) # 0.24 def test_check_estimator(): # tests that the estimator actually fails on "bad" estimators. # not a complete test of all checks, which are very extensive. @@ -580,9 +580,10 @@ def test_check_regressor_data_not_an_array(): EstimatorInconsistentForPandas()) -@ignore_warnings("Passing a class is depr", category=FutureWarning) # 0.25 +@ignore_warnings("Passing a class is depr", category=FutureWarning) # 0.24 def test_check_estimator_required_parameters_skip(): - # TODO: remove whole test in 0.25 + # TODO: remove whole test in 0.24 since passes classes to check_estimator() + # isn't supported anymore class MyEstimator(BaseEstimator): _required_parameters = ["special_parameter"] From e7396dbf2a6db6833439df5bcb9e2641b2754217 Mon Sep 17 00:00:00 2001 From: Nicolas Hug Date: Mon, 27 Apr 2020 10:24:38 -0400 Subject: [PATCH 11/12] Added whatsnew entry --- doc/whats_new/v0.23.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/doc/whats_new/v0.23.rst b/doc/whats_new/v0.23.rst index 4357845885e3f..5aaf62ad318ed 100644 --- a/doc/whats_new/v0.23.rst +++ b/doc/whats_new/v0.23.rst @@ -511,6 +511,11 @@ Changelog matrix from a pandas DataFrame that contains only `SparseArray`s. :pr:`16728` by `Thomas Fan`_. +- |API| Passing classes to :func:`utils.estimator_checks.check_estimator` and + :func:`utils.estimator_checks.parametrize_with_checks` is now deprecated, + and support for classe will be removed in 0.24. Pass instances instead. + :pr:`17032` by `Nicolas Hug`_. + :mod:`sklearn.cluster` ...................... From 268728ba1f78aedc872c0a521d5de2b2c469cc9e Mon Sep 17 00:00:00 2001 From: Thomas J Fan Date: Mon, 27 Apr 2020 15:19:39 -0400 Subject: [PATCH 12/12] CLN Address comments --- doc/whats_new/v0.23.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/whats_new/v0.23.rst b/doc/whats_new/v0.23.rst index 5aaf62ad318ed..f9b8e25176265 100644 --- a/doc/whats_new/v0.23.rst +++ b/doc/whats_new/v0.23.rst @@ -513,7 +513,7 @@ Changelog - |API| Passing classes to :func:`utils.estimator_checks.check_estimator` and :func:`utils.estimator_checks.parametrize_with_checks` is now deprecated, - and support for classe will be removed in 0.24. Pass instances instead. + and support for classes will be removed in 0.24. Pass instances instead. :pr:`17032` by `Nicolas Hug`_. :mod:`sklearn.cluster`