From dc702161d996ed6e3657d8ea643f0f88b74eb1ec Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Wed, 3 Aug 2022 17:55:40 +0200 Subject: [PATCH 01/20] Make CalibratedClassifierCV work with SLEP006 This PR adds metadata routing to CalibratedClassifierCV (CCV). CCV uses a subestimator to create (out of sample) probabilities, which are in turn used to calibrate the probabilities. The metaestimator uses sample_weight. The subestimator may or may not use sample_weight and additional metadata. So far, it was checked if the subestimator has sample_weight in its signature and then they were routed, otherwise not. This is, however, not always ideal, e.g. when the subestimator is itself a pipeline (https://github.com/scikit-learn/scikit-learn/issues/21134). With routing, this problem disappears. In addition to these changes, the tests in test_metaestimator_metadata_routing.py have been amended to make them more generic, as right now, they are specific to multioutput. --- sklearn/calibration.py | 124 ++++---- sklearn/tests/test_calibration.py | 42 +-- .../test_metaestimators_metadata_routing.py | 283 ++++++++++++++---- sklearn/utils/_metadata_requests.py | 9 +- sklearn/utils/_mocking.py | 9 + 5 files changed, 324 insertions(+), 143 deletions(-) diff --git a/sklearn/calibration.py b/sklearn/calibration.py index d5eba5a761ebe..990ac3ed5a06d 100644 --- a/sklearn/calibration.py +++ b/sklearn/calibration.py @@ -8,7 +8,6 @@ # License: BSD 3 clause import warnings -from inspect import signature from functools import partial from math import log @@ -49,6 +48,7 @@ from .model_selection import check_cv, cross_val_predict from .metrics._base import _check_pos_label_consistency from .metrics._plot.base import _get_response +from .utils.metadata_routing import MetadataRouter, MethodMapping, process_routing class CalibratedClassifierCV(ClassifierMixin, MetaEstimatorMixin, BaseEstimator): @@ -259,6 +259,31 @@ def __init__( self.ensemble = ensemble self.base_estimator = base_estimator + def _get_estimator(self): + """Resolve which estimator to return (default is LinearSVC)""" + # TODO(1.4): Remove when base_estimator is removed + if self.base_estimator != "deprecated": + if self.estimator is not None: + raise ValueError( + "Both `base_estimator` and `estimator` are set. Only set " + "`estimator` since `base_estimator` is deprecated." + ) + warnings.warn( + "`base_estimator` was renamed to `estimator` in version 1.2 and " + "will be removed in 1.4.", + FutureWarning, + ) + estimator = self.base_estimator + else: + estimator = self.estimator + + if estimator is None: + # we want all classifiers that don't expose a random_state + # to be deterministic (and we don't want to expose this one). + estimator = LinearSVC(random_state=0).set_fit_request(sample_weight=True) + + return estimator + def fit(self, X, y, sample_weight=None, **fit_params): """Fit the calibrated model. @@ -290,26 +315,7 @@ def fit(self, X, y, sample_weight=None, **fit_params): for sample_aligned_params in fit_params.values(): check_consistent_length(y, sample_aligned_params) - # TODO(1.4): Remove when base_estimator is removed - if self.base_estimator != "deprecated": - if self.estimator is not None: - raise ValueError( - "Both `base_estimator` and `estimator` are set. Only set " - "`estimator` since `base_estimator` is deprecated." - ) - warnings.warn( - "`base_estimator` was renamed to `estimator` in version 1.2 and " - "will be removed in 1.4.", - FutureWarning, - ) - estimator = self.base_estimator - else: - estimator = self.estimator - - if estimator is None: - # we want all classifiers that don't expose a random_state - # to be deterministic (and we don't want to expose this one). - estimator = LinearSVC(random_state=0) + estimator = self._get_estimator() self.calibrated_classifiers_ = [] if self.cv == "prefit": @@ -336,20 +342,12 @@ def fit(self, X, y, sample_weight=None, **fit_params): self.classes_ = label_encoder_.classes_ n_classes = len(self.classes_) - # sample_weight checks - fit_parameters = signature(estimator.fit).parameters - supports_sw = "sample_weight" in fit_parameters - if sample_weight is not None and not supports_sw: - estimator_name = type(estimator).__name__ - warnings.warn( - f"Since {estimator_name} does not appear to accept sample_weight, " - "sample weights will only be used for the calibration itself. This " - "can be caused by a limitation of the current scikit-learn API. " - "See the following issue for more details: " - "https://github.com/scikit-learn/scikit-learn/issues/21134. Be " - "warned that the result of the calibration is likely to be " - "incorrect." - ) + routed_params = process_routing( + obj=self, + method="fit", + sample_weight=sample_weight, + other_params=fit_params, + ) # Check that each cross-validation fold can have at least one # example per class @@ -380,20 +378,14 @@ def fit(self, X, y, sample_weight=None, **fit_params): test=test, method=self.method, classes=self.classes_, - supports_sw=supports_sw, sample_weight=sample_weight, - **fit_params, + fit_params=routed_params.estimator.fit, ) for train, test in cv.split(X, y) ) else: this_estimator = clone(estimator) _, method_name = _get_prediction_method(this_estimator) - fit_params = ( - {"sample_weight": sample_weight} - if sample_weight is not None and supports_sw - else None - ) pred_method = partial( cross_val_predict, estimator=this_estimator, @@ -402,16 +394,13 @@ def fit(self, X, y, sample_weight=None, **fit_params): cv=cv, method=method_name, n_jobs=self.n_jobs, - fit_params=fit_params, + fit_params=routed_params.estimator.fit, ) predictions = _compute_predictions( pred_method, method_name, X, n_classes ) - if sample_weight is not None and supports_sw: - this_estimator.fit(X, y, sample_weight=sample_weight) - else: - this_estimator.fit(X, y) + this_estimator.fit(X, y, **routed_params.estimator.fit) # Note: Here we don't pass on fit_params because the supported # calibrators don't support fit_params anyway calibrated_classifier = _fit_calibrator( @@ -478,6 +467,33 @@ def predict(self, X): check_is_fitted(self) return self.classes_[np.argmax(self.predict_proba(X), axis=1)] + def get_metadata_routing(self): + """Get metadata routing of this object. + + Please check :ref:`User Guide ` on how the routing + mechanism works. + + Returns + ------- + routing : MetadataRouter + A :class:`~utils.metadata_routing.MetadataRouter` encapsulating + routing information. + """ + router = ( + MetadataRouter(owner=self.__class__.__name__) + .add_self(self) + .add( + estimator=self._get_estimator(), + method_mapping=MethodMapping().add(callee="fit", caller="fit"), + ) + # the fit method already accepts everything, therefore we don't + # specify parameters. The value passed to ``child`` needs to be the + # same as what's passed to ``add`` above, in this case + # `"estimator"`. + .warn_on(child="estimator", method="fit", params=None) + ) + return router + def _more_tags(self): return { "_xfail_checks": { @@ -496,11 +512,10 @@ def _fit_classifier_calibrator_pair( y, train, test, - supports_sw, method, classes, sample_weight=None, - **fit_params, + fit_params=None, ): """Fit a classifier/calibration pair on a given train/test split. @@ -525,9 +540,6 @@ def _fit_classifier_calibrator_pair( test : ndarray, shape (n_test_indices,) Indices of the testing subset. - supports_sw : bool - Whether or not the `estimator` supports sample weights. - method : {'sigmoid', 'isotonic'} Method to use for calibration. @@ -537,7 +549,7 @@ def _fit_classifier_calibrator_pair( sample_weight : array-like, default=None Sample weights for `X`. - **fit_params : dict + fit_params : dict, default=None Parameters to pass to the `fit` method of the underlying classifier. @@ -549,11 +561,7 @@ def _fit_classifier_calibrator_pair( X_train, y_train = _safe_indexing(X, train), _safe_indexing(y, train) X_test, y_test = _safe_indexing(X, test), _safe_indexing(y, test) - if sample_weight is not None and supports_sw: - sw_train = _safe_indexing(sample_weight, train) - estimator.fit(X_train, y_train, sample_weight=sw_train, **fit_params_train) - else: - estimator.fit(X_train, y_train, **fit_params_train) + estimator.fit(X_train, y_train, **fit_params_train) n_classes = len(classes) pred_method, method_name = _get_prediction_method(estimator) diff --git a/sklearn/tests/test_calibration.py b/sklearn/tests/test_calibration.py index cb404b4c73199..12941dafc5984 100644 --- a/sklearn/tests/test_calibration.py +++ b/sklearn/tests/test_calibration.py @@ -50,6 +50,10 @@ N_SAMPLES = 200 +def _weighted(estimator): + return estimator.set_fit_request(sample_weight=True) + + @pytest.fixture(scope="module") def data(): X, y = make_classification(n_samples=N_SAMPLES, n_features=6, random_state=42) @@ -83,7 +87,9 @@ def test_calibration(data, method, ensemble): (X_train, X_test), (sparse.csr_matrix(X_train), sparse.csr_matrix(X_test)), ]: - cal_clf = CalibratedClassifierCV(clf, method=method, cv=5, ensemble=ensemble) + cal_clf = CalibratedClassifierCV( + _weighted(clf), method=method, cv=5, ensemble=ensemble + ) # Note that this fit overwrites the fit on the entire training # set cal_clf.fit(this_X_train, y_train, sample_weight=sw_train) @@ -175,7 +181,7 @@ def test_sample_weight(data, method, ensemble): X_train, y_train, sw_train = X[:n_samples], y[:n_samples], sample_weight[:n_samples] X_test = X[n_samples:] - estimator = LinearSVC(random_state=42) + estimator = _weighted(LinearSVC(random_state=42)) calibrated_clf = CalibratedClassifierCV(estimator, method=method, ensemble=ensemble) calibrated_clf.fit(X_train, y_train, sample_weight=sw_train) probs_with_sw = calibrated_clf.predict_proba(X_test) @@ -909,7 +915,7 @@ def test_calibrated_classifier_cv_double_sample_weights_equivalence(method, ense y_twice[::2] = y y_twice[1::2] = y - estimator = LogisticRegression() + estimator = _weighted(LogisticRegression()) calibrated_clf_without_weights = CalibratedClassifierCV( estimator, method=method, @@ -951,7 +957,9 @@ def test_calibration_with_fit_params(fit_params_type, data): "b": _convert_container(y, fit_params_type), } - clf = CheckingClassifier(expected_fit_params=["a", "b"]) + clf = CheckingClassifier(expected_fit_params=["a", "b"]).set_fit_request( + a=True, b=True + ) pc_clf = CalibratedClassifierCV(clf) pc_clf.fit(X, y, **fit_params) @@ -969,34 +977,12 @@ def test_calibration_with_sample_weight_base_estimator(sample_weight, data): estimator. """ X, y = data - clf = CheckingClassifier(expected_sample_weight=True) + clf = _weighted(CheckingClassifier(expected_sample_weight=True)) pc_clf = CalibratedClassifierCV(clf) pc_clf.fit(X, y, sample_weight=sample_weight) -def test_calibration_without_sample_weight_base_estimator(data): - """Check that even if the estimator doesn't support - sample_weight, fitting with sample_weight still works. - - There should be a warning, since the sample_weight is not passed - on to the estimator. - """ - X, y = data - sample_weight = np.ones_like(y) - - class ClfWithoutSampleWeight(CheckingClassifier): - def fit(self, X, y, **fit_params): - assert "sample_weight" not in fit_params - return super().fit(X, y, **fit_params) - - clf = ClfWithoutSampleWeight() - pc_clf = CalibratedClassifierCV(clf) - - with pytest.warns(UserWarning): - pc_clf.fit(X, y, sample_weight=sample_weight) - - def test_calibration_with_fit_params_inconsistent_length(data): """fit_params having different length than data should raise the correct error message. @@ -1029,7 +1015,7 @@ def test_calibrated_classifier_cv_zeros_sample_weights_equivalence(method, ensem sample_weight = np.zeros_like(y) sample_weight[::2] = 1 - estimator = LogisticRegression() + estimator = _weighted(LogisticRegression()) calibrated_clf_without_weights = CalibratedClassifierCV( estimator, method=method, diff --git a/sklearn/tests/test_metaestimators_metadata_routing.py b/sklearn/tests/test_metaestimators_metadata_routing.py index dae9fbf3aa03a..5b4059062d91d 100644 --- a/sklearn/tests/test_metaestimators_metadata_routing.py +++ b/sklearn/tests/test_metaestimators_metadata_routing.py @@ -1,6 +1,11 @@ +import re +import warnings + import numpy as np import pytest from sklearn.base import RegressorMixin, ClassifierMixin, BaseEstimator +from sklearn.calibration import CalibratedClassifierCV +from sklearn.exceptions import UnsetMetadataPassedError from sklearn.multioutput import MultiOutputRegressor, MultiOutputClassifier from sklearn.utils.metadata_routing import MetadataRouter from sklearn.tests.test_metadata_routing import ( @@ -17,109 +22,277 @@ sample_weight = np.random.rand(N) +class _Registry(list): + # This list is used to get a reference to the sub-estimators, which are not + # necessarily stored on the metaestimator. We need to override __deepcopy__ + # because the sub-estimators are probably cloned, which would result in a + # new copy of the list. + def __deepcopy__(self, memo): + return self + + class ConsumingRegressor(RegressorMixin, BaseEstimator): - """A regressor consuming metadata.""" + """A regressor consuming metadata. + + Parameters + ---------- + registry : list or None, default=None + If a list, that estimator will append itself to the list in order to + have a reference to the estimator later on. Since that reference is not + required in all tests, registration can be skipped by leaving this value + as None. + + """ + def __init__(self, registry=None): + self.registry = registry def partial_fit(self, X, y, sample_weight=None, metadata=None): + if self.registry is not None: + self.registry.append(self) + record_metadata( self, "partial_fit", sample_weight=sample_weight, metadata=metadata ) return self def fit(self, X, y, sample_weight=None, metadata=None): + if self.registry is not None: + self.registry.append(self) + record_metadata(self, "fit", sample_weight=sample_weight, metadata=metadata) return self - def predict(self, X, y, sample_weight=None, metadata=None): + def predict(self, X, sample_weight=None, metadata=None): + if self.registry is not None: + self.registry.append(self) + record_metadata(self, "predict", sample_weight=sample_weight, metadata=metadata) - return np.zeros(shape=(len(X))) + return np.zeros(shape=(len(X),)) class ConsumingClassifier(ClassifierMixin, BaseEstimator): - """A classifier consuming metadata.""" + """A classifier consuming metadata. + + Parameters + ---------- + registry : list or None, default=None + If a list, that estimator will append itself to the list in order to + have a reference to the estimator later on. Since that reference is not + required in all tests, registration can be skipped by leaving this value + as None. + + """ + def __init__(self, registry=None): + self.registry = registry def partial_fit(self, X, y, sample_weight=None, metadata=None): + if self.registry is not None: + self.registry.append(self) + record_metadata( self, "partial_fit", sample_weight=sample_weight, metadata=metadata ) - self.classes_ = [1] + self.classes_ = [0, 1] return self def fit(self, X, y, sample_weight=None, metadata=None): + if self.registry is not None: + self.registry.append(self) + record_metadata(self, "fit", sample_weight=sample_weight, metadata=metadata) - self.classes_ = [1] + self.classes_ = [0, 1] return self - def predict(self, X, y, sample_weight=None, metadata=None): + def predict(self, X, sample_weight=None, metadata=None): + if self.registry is not None: + self.registry.append(self) + record_metadata(self, "predict", sample_weight=sample_weight, metadata=metadata) - return np.zeros(shape=(len(X))) + return np.zeros(shape=(len(X),)) + + def predict_proba(self, X, sample_weight=None, metadata=None): + if self.registry is not None: + self.registry.append(self) - def predict_proba(self, X, y, sample_weight=None, metadata=None): record_metadata( self, "predict_proba", sample_weight=sample_weight, metadata=metadata ) - return np.zeros(shape=(len(X))) + return np.asarray([[0.0, 1.0]] * len(X)) + + def predict_log_proba(self, X, sample_weight=None, metadata=None): + if self.registry is not None: + self.registry.append(self) - def predict_log_proba(self, X, y, sample_weight=None, metadata=None): record_metadata( self, "predict_log_proba", sample_weight=sample_weight, metadata=metadata ) - return np.zeros(shape=(len(X))) + return np.zeros(shape=(len(X), 2)) + + +METAESTIMATORS = [ + { + "metaestimator": MultiOutputRegressor, + "estimator_name": "estimator", + "estimator": ConsumingRegressor, + "X": X, + "y": y_multi, + "routing_methods": ["fit", "partial_fit"], + "fit": {"warns_on": ["sample_weight"]}, + }, + { + "metaestimator": MultiOutputClassifier, + "estimator_name": "estimator", + "estimator": ConsumingClassifier, + "X": X, + "y": y_multi, + "routing_methods": ["fit", "partial_fit"], + "fit": {"warns_on": ["sample_weight"]}, + }, + { + "metaestimator": CalibratedClassifierCV, + "estimator_name": "estimator", + "estimator": ConsumingClassifier, + "X": X, + "y": y, + "routing_methods": ["fit"], + "fit": {"warns_on": "all"}, + "preserves_metadata": False, + }, +] +"""List containing all metaestimators to be tested and their settings + +The keys are as follows: +- metaestimator: The metaestmator to be tested +- estimator_name: The name of the argument for the sub-estimator +- estimator: The sub-estimator +- X: X-data to fit and predict +- y: y-data to fit +- routing_methods: list of all methods to check for routing +- : How said method behaves, e.g. "fit": {"warns_on": + ["sample_weight"]} means fit warns when sample weights are passed but not + requested. +- preserves_metadata: Whether the metaestimator passes the metadata to the + sub-estimator without modification or not. If it does, we check that the + values are identical. If it doesn', no check is performed. TODO Maybe + something smarter could be done if the data is modified. -def get_empty_metaestimators(): - yield MultiOutputRegressor(estimator=ConsumingRegressor()) - yield MultiOutputClassifier(estimator=ConsumingClassifier()) +""" @pytest.mark.parametrize( "metaestimator", - get_empty_metaestimators(), - ids=[str(x) for x in get_empty_metaestimators()], + METAESTIMATORS, + ids=[str(row["metaestimator"].__class__.__name__) for row in METAESTIMATORS], ) def test_default_request(metaestimator): # Check that by default request is empty and the right type - assert_request_is_empty(metaestimator.get_metadata_routing()) - assert isinstance(metaestimator.get_metadata_routing(), MetadataRouter) + cls = metaestimator["metaestimator"] + estimator = metaestimator["estimator"]() + estimator_name = metaestimator["estimator_name"] + instance = cls(**{estimator_name: estimator}) + assert_request_is_empty(instance.get_metadata_routing()) + assert isinstance(instance.get_metadata_routing(), MetadataRouter) @pytest.mark.parametrize( - "MultiOutput, Estimator", - [ - (MultiOutputClassifier, ConsumingClassifier), - (MultiOutputRegressor, ConsumingRegressor), - ], - ids=["Classifier", "Regressor"], + "metaestimator", + METAESTIMATORS, + ids=[str(row["metaestimator"].__class__.__name__) for row in METAESTIMATORS], +) +def test_warning_for_indicated_methods(metaestimator): + # Check that the indicated methods give a warning + # TODO: After deprecation period, always error + cls = metaestimator["metaestimator"] + registry = _Registry() + estimator = metaestimator["estimator"](registry=registry) + estimator_name = metaestimator["estimator_name"] + instance = cls(**{estimator_name: estimator}) + X = metaestimator["X"] + y = metaestimator["y"] + routing_methods = metaestimator["routing_methods"] + + for method_name in routing_methods: + if not metaestimator.get(method_name, {}).get("warns_on"): + # this method is not expected to warn + continue + + warn_msg = ( + "You are passing metadata for which the request values are not explicitly " + "set: sample_weight, metadata. From version 1.4 this results in the " + "following error: [sample_weight, metadata] are passed but are not " + "explicitly set as requested or not for " + f"{estimator.__class__.__name__}.{method_name}" + ) + with pytest.warns(FutureWarning, match=re.escape(warn_msg)): + method = getattr(instance, method_name) + method(X, y, sample_weight=sample_weight, metadata=metadata) + + if metaestimator.get("preserves_metadata", True): + check_recorded_metadata( + registry[-1], + method_name, + sample_weight=sample_weight, + metadata=metadata, + ) + + +@pytest.mark.parametrize( + "metaestimator", + METAESTIMATORS, + ids=[str(row["metaestimator"].__class__.__name__) for row in METAESTIMATORS], ) -def test_multioutput_metadata_routing(MultiOutput, Estimator): - # Check routing of metadata - metaest = MultiOutput(Estimator()) - warn_msg = ( - "You are passing metadata for which the request values are not explicitly" - " set: sample_weight, metadata." - ) - with pytest.warns(FutureWarning, match=(warn_msg)): - metaest.fit(X, y_multi, sample_weight=sample_weight, metadata=metadata) - check_recorded_metadata( - metaest.estimators_[0], - "fit", - sample_weight=sample_weight, - metadata=metadata, +def test_error_for_other_methods(metaestimator): + # This test complements test_warning_for_indicated_methods but checks for + # UnsetMetadataPassedError instead of FutureWarning + cls = metaestimator["metaestimator"] + estimator = metaestimator["estimator"]() + estimator_name = metaestimator["estimator_name"] + instance = cls(**{estimator_name: estimator}) + X = metaestimator["X"] + y = metaestimator["y"] + routing_methods = metaestimator["routing_methods"] + + for method_name in routing_methods: + if metaestimator.get(method_name, {}).get("warns_on"): + # this method is not expected to warn + continue + + msg = ( + "[sample_weight, metadata] are passed but are not explicitly set as " + "requested or not for " + f"{estimator.__class__.__name__}.{method_name}" ) + with pytest.raises(UnsetMetadataPassedError, match=re.escape(msg)): + method = getattr(instance, method_name) + method(X, y, sample_weight=sample_weight, metadata=metadata) + + +@pytest.mark.parametrize( + "metaestimator", + METAESTIMATORS, + ids=[str(row["metaestimator"].__class__.__name__) for row in METAESTIMATORS], +) +def test_setting_request_removes_warning_or_error(metaestimator): + # When the metadata is explicitly requested, there should be no warning and + # no error. + def set_request(estimator, method_name): + # e.g. call set_fit_request on estimator + method = getattr(estimator, f"set_{method_name}_request") + method(sample_weight=True, metadata=True) + + cls = metaestimator["metaestimator"] + estimator_name = metaestimator["estimator_name"] + X = metaestimator["X"] + y = metaestimator["y"] + routing_methods = metaestimator["routing_methods"] - metaest = MultiOutput( - Estimator() - .set_fit_request(sample_weight=True, metadata="alias") - .set_partial_fit_request(sample_weight=True, metadata=True) - ).fit(X, y_multi, alias=metadata) - # if an estimator requests a metadata but it's not passed, no errors are - # raised. Therefore here we don't pass `sample_weight` to test nothing is - # raised. - check_recorded_metadata( - metaest.estimators_[0], "fit", sample_weight=None, metadata=metadata - ) - - metaest.partial_fit(X, y_multi, metadata=metadata) - check_recorded_metadata( - metaest.estimators_[0], "partial_fit", sample_weight=None, metadata=metadata - ) + for method_name in routing_methods: + estimator = metaestimator["estimator"]() + set_request(estimator, method_name) + instance = cls(**{estimator_name: estimator}) + # lines below to ensure that there are no warnings + with warnings.catch_warnings(): + warnings.simplefilter("error") + method = getattr(instance, method_name) + method(X, y, sample_weight=sample_weight, metadata=metadata) diff --git a/sklearn/utils/_metadata_requests.py b/sklearn/utils/_metadata_requests.py index a8ec3ffb8db77..297c08c8c621b 100644 --- a/sklearn/utils/_metadata_requests.py +++ b/sklearn/utils/_metadata_requests.py @@ -1051,6 +1051,10 @@ class RequestMethod: function, e.g. ``["sample_weight"]`` if the corresponding method accepts it as a metadata. + validate_keys : bool, default=True + Whether to check if the requested parameters fit the actual parameters + of the method. + Notes ----- This class is a descriptor [1]_ and uses PEP-362 to set the signature of @@ -1063,9 +1067,10 @@ class RequestMethod: .. [2] https://www.python.org/dev/peps/pep-0362/ """ - def __init__(self, name, keys): + def __init__(self, name, keys, validate_keys=True): self.name = name self.keys = keys + self.validate_keys = validate_keys def __get__(self, instance, owner): # we would want to have a method which accepts only the expected args @@ -1075,7 +1080,7 @@ def func(**kw): This docstring is overwritten below. See REQUESTER_DOC for expected functionality """ - if set(kw) - set(self.keys): + if self.validate_keys and (set(kw) - set(self.keys)): raise TypeError( f"Unexpected args: {set(kw) - set(self.keys)}. Accepted arguments" f" are: {set(self.keys)}" diff --git a/sklearn/utils/_mocking.py b/sklearn/utils/_mocking.py index c7451dce1fbc5..cc73e407a89b0 100644 --- a/sklearn/utils/_mocking.py +++ b/sklearn/utils/_mocking.py @@ -3,6 +3,7 @@ from ..base import BaseEstimator, ClassifierMixin from .validation import _check_sample_weight, _num_samples, check_array from .validation import check_is_fitted +from ..utils._metadata_requests import RequestMethod class ArraySlicingWrapper: @@ -321,6 +322,14 @@ def _more_tags(self): return {"_skip_test": True, "X_types": ["1dlabel"]} +# Deactivate key validation for CheckingClassifier because we want to be able to +# call fit with arbitrary fit_params and record them. Without this change, we +# would get an error because those arbitrary params are not expected. +CheckingClassifier.set_fit_request = RequestMethod( # noqa + name="fit", keys=[], validate_keys=False +) + + class NoSampleWeightWrapper(BaseEstimator): """Wrap estimator which will not expose `sample_weight`. From 0d629452a899eff9166a2c7f5540897f5bacf91c Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Fri, 5 Aug 2022 18:09:11 +0200 Subject: [PATCH 02/20] Fix black complaints --- sklearn/tests/test_metaestimators_metadata_routing.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sklearn/tests/test_metaestimators_metadata_routing.py b/sklearn/tests/test_metaestimators_metadata_routing.py index 5b4059062d91d..62a458ce738cd 100644 --- a/sklearn/tests/test_metaestimators_metadata_routing.py +++ b/sklearn/tests/test_metaestimators_metadata_routing.py @@ -43,6 +43,7 @@ class ConsumingRegressor(RegressorMixin, BaseEstimator): as None. """ + def __init__(self, registry=None): self.registry = registry @@ -82,6 +83,7 @@ class ConsumingClassifier(ClassifierMixin, BaseEstimator): as None. """ + def __init__(self, registry=None): self.registry = registry From 55488ca362be89c7f63f042f209c7855c44a0a00 Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Mon, 8 Aug 2022 16:19:40 +0200 Subject: [PATCH 03/20] Address reviewer comments by Adrin - Add __copy__ method to Registry - Fix parameter docstrings - Don't repeat metaestimator ids code - Check_recorded_metadata runs for all registered estimators - More fine-grained check for warnings, so as _not_ to error on unrelated warnings --- .../test_metaestimators_metadata_routing.py | 56 ++++++++++++------- 1 file changed, 37 insertions(+), 19 deletions(-) diff --git a/sklearn/tests/test_metaestimators_metadata_routing.py b/sklearn/tests/test_metaestimators_metadata_routing.py index 62a458ce738cd..c93abc371f731 100644 --- a/sklearn/tests/test_metaestimators_metadata_routing.py +++ b/sklearn/tests/test_metaestimators_metadata_routing.py @@ -30,15 +30,18 @@ class _Registry(list): def __deepcopy__(self, memo): return self + def __copy__(self, memo): + return self + class ConsumingRegressor(RegressorMixin, BaseEstimator): """A regressor consuming metadata. Parameters ---------- - registry : list or None, default=None - If a list, that estimator will append itself to the list in order to - have a reference to the estimator later on. Since that reference is not + registry : list, default=None + If a list, the estimator will append itself to the list in order to have + a reference to the estimator later on. Since that reference is not required in all tests, registration can be skipped by leaving this value as None. @@ -76,9 +79,9 @@ class ConsumingClassifier(ClassifierMixin, BaseEstimator): Parameters ---------- - registry : list or None, default=None - If a list, that estimator will append itself to the list in order to - have a reference to the estimator later on. Since that reference is not + registry : list, default=None + If a list, the estimator will append itself to the list in order to have + a reference to the estimator later on. Since that reference is not required in all tests, registration can be skipped by leaving this value as None. @@ -181,11 +184,16 @@ def predict_log_proba(self, X, sample_weight=None, metadata=None): """ +# ids used for pytest fixture +METAESTIMATOR_IDS = [ + str(row["metaestimator"].__class__.__name__) for row in METAESTIMATORS +] + @pytest.mark.parametrize( "metaestimator", METAESTIMATORS, - ids=[str(row["metaestimator"].__class__.__name__) for row in METAESTIMATORS], + ids=METAESTIMATOR_IDS, ) def test_default_request(metaestimator): # Check that by default request is empty and the right type @@ -200,11 +208,11 @@ def test_default_request(metaestimator): @pytest.mark.parametrize( "metaestimator", METAESTIMATORS, - ids=[str(row["metaestimator"].__class__.__name__) for row in METAESTIMATORS], + ids=METAESTIMATOR_IDS, ) def test_warning_for_indicated_methods(metaestimator): # Check that the indicated methods give a warning - # TODO: After deprecation period, always error + # TODO: Always error for 1.4 cls = metaestimator["metaestimator"] registry = _Registry() estimator = metaestimator["estimator"](registry=registry) @@ -231,18 +239,24 @@ def test_warning_for_indicated_methods(metaestimator): method(X, y, sample_weight=sample_weight, metadata=metadata) if metaestimator.get("preserves_metadata", True): - check_recorded_metadata( - registry[-1], - method_name, - sample_weight=sample_weight, - metadata=metadata, - ) + # sanity check that registry is not empty, or else the test passes trivially + assert registry + for estimator in registry: + check_recorded_metadata( + estimator, + method_name, + sample_weight=sample_weight, + metadata=metadata, + ) + # clear the registry since the check could be different for the next + # method being tested + registry.clear() @pytest.mark.parametrize( "metaestimator", METAESTIMATORS, - ids=[str(row["metaestimator"].__class__.__name__) for row in METAESTIMATORS], + ids=METAESTIMATOR_IDS, ) def test_error_for_other_methods(metaestimator): # This test complements test_warning_for_indicated_methods but checks for @@ -273,7 +287,7 @@ def test_error_for_other_methods(metaestimator): @pytest.mark.parametrize( "metaestimator", METAESTIMATORS, - ids=[str(row["metaestimator"].__class__.__name__) for row in METAESTIMATORS], + ids=METAESTIMATOR_IDS, ) def test_setting_request_removes_warning_or_error(metaestimator): # When the metadata is explicitly requested, there should be no warning and @@ -294,7 +308,11 @@ def set_request(estimator, method_name): set_request(estimator, method_name) instance = cls(**{estimator_name: estimator}) # lines below to ensure that there are no warnings - with warnings.catch_warnings(): - warnings.simplefilter("error") + with warnings.catch_warnings(record=True) as rec: method = getattr(instance, method_name) method(X, y, sample_weight=sample_weight, metadata=metadata) + # Check that there was no FutureWarning about metadata. The exact + # error message is not checked on purpose, because if the message is + # changed without amending this test, the test would pass trivially. + future_warnings = [w for w in rec if isinstance(w, FutureWarning)] + assert not any("metadata" in w.message for w in future_warnings) From f81389a19b1364746ae9b0f46816ed89f151fbfd Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Tue, 9 Aug 2022 14:25:38 +0200 Subject: [PATCH 04/20] Address reviewer comment: warns_on Change around structure of the generic test to make more sense. Use the values to check for specific arguments that should be warned on, instead of all arguments at once. --- sklearn/tests/test_metadata_routing.py | 6 +- .../test_metaestimators_metadata_routing.py | 70 +++++++++---------- 2 files changed, 38 insertions(+), 38 deletions(-) diff --git a/sklearn/tests/test_metadata_routing.py b/sklearn/tests/test_metadata_routing.py index a1bc083ef2da8..77e7c20fd8ba7 100644 --- a/sklearn/tests/test_metadata_routing.py +++ b/sklearn/tests/test_metadata_routing.py @@ -73,14 +73,16 @@ def assert_request_equal(request, dictionary): def record_metadata(obj, method, **kwargs): """Utility function to store passed metadata to a method.""" if not hasattr(obj, "_records"): - setattr(obj, "_records", dict()) + obj._records = {} obj._records[method] = kwargs def check_recorded_metadata(obj, method, **kwargs): """Check whether the expected metadata is passed to the object's method.""" records = getattr(obj, "_records", dict()).get(method, dict()) - assert set(kwargs.keys()) == set(records.keys()) + # only check keys whose value was explicitly passed + expected_keys = {key for key, val in records.items() if val is not None} + assert set(kwargs.keys()) == expected_keys for key, value in kwargs.items(): assert records[key] is value diff --git a/sklearn/tests/test_metaestimators_metadata_routing.py b/sklearn/tests/test_metaestimators_metadata_routing.py index c93abc371f731..eda1b71b5974e 100644 --- a/sklearn/tests/test_metaestimators_metadata_routing.py +++ b/sklearn/tests/test_metaestimators_metadata_routing.py @@ -142,7 +142,7 @@ def predict_log_proba(self, X, sample_weight=None, metadata=None): "X": X, "y": y_multi, "routing_methods": ["fit", "partial_fit"], - "fit": {"warns_on": ["sample_weight"]}, + "warns_on": {"fit": ["sample_weight"]}, }, { "metaestimator": MultiOutputClassifier, @@ -151,7 +151,7 @@ def predict_log_proba(self, X, sample_weight=None, metadata=None): "X": X, "y": y_multi, "routing_methods": ["fit", "partial_fit"], - "fit": {"warns_on": ["sample_weight"]}, + "warns_on": {"fit": ["sample_weight"]}, }, { "metaestimator": CalibratedClassifierCV, @@ -160,7 +160,7 @@ def predict_log_proba(self, X, sample_weight=None, metadata=None): "X": X, "y": y, "routing_methods": ["fit"], - "fit": {"warns_on": "all"}, + "warns_on": {"fit": ["sample_weight", "metadata"]}, "preserves_metadata": False, }, ] @@ -174,9 +174,9 @@ def predict_log_proba(self, X, sample_weight=None, metadata=None): - X: X-data to fit and predict - y: y-data to fit - routing_methods: list of all methods to check for routing -- : How said method behaves, e.g. "fit": {"warns_on": - ["sample_weight"]} means fit warns when sample weights are passed but not - requested. +- warns_on: A dict containing all methods as keys that are supposed to result in + a warning if routing is not requested. It is implied that all routing methods + not listed here should result in an error. - preserves_metadata: Whether the metaestimator passes the metadata to the sub-estimator without modification or not. If it does, we check that the values are identical. If it doesn', no check is performed. TODO Maybe @@ -185,9 +185,7 @@ def predict_log_proba(self, X, sample_weight=None, metadata=None): """ # ids used for pytest fixture -METAESTIMATOR_IDS = [ - str(row["metaestimator"].__class__.__name__) for row in METAESTIMATORS -] +METAESTIMATOR_IDS = [str(row["metaestimator"].__name__) for row in METAESTIMATORS] @pytest.mark.parametrize( @@ -221,36 +219,35 @@ def test_warning_for_indicated_methods(metaestimator): X = metaestimator["X"] y = metaestimator["y"] routing_methods = metaestimator["routing_methods"] + warns_on = metaestimator["warns_on"] for method_name in routing_methods: - if not metaestimator.get(method_name, {}).get("warns_on"): + if method_name not in warns_on: # this method is not expected to warn continue - warn_msg = ( - "You are passing metadata for which the request values are not explicitly " - "set: sample_weight, metadata. From version 1.4 this results in the " - "following error: [sample_weight, metadata] are passed but are not " - "explicitly set as requested or not for " - f"{estimator.__class__.__name__}.{method_name}" - ) - with pytest.warns(FutureWarning, match=re.escape(warn_msg)): - method = getattr(instance, method_name) - method(X, y, sample_weight=sample_weight, metadata=metadata) - - if metaestimator.get("preserves_metadata", True): - # sanity check that registry is not empty, or else the test passes trivially - assert registry - for estimator in registry: - check_recorded_metadata( - estimator, - method_name, - sample_weight=sample_weight, - metadata=metadata, - ) - # clear the registry since the check could be different for the next - # method being tested - registry.clear() + for key in warns_on[method_name]: + val = {"sample_weight": sample_weight, "metadata": metadata}[key] + kwargs = {key: val} + warn_msg = ( + "You are passing metadata for which the request values are not" + f" explicitly set: {key}. From version 1.4 this results in the" + f" following error: [{key}] are passed but are not explicitly set as" + f" requested or not for {estimator.__class__.__name__}.{method_name}" + ) + with pytest.warns(FutureWarning, match=re.escape(warn_msg)): + method = getattr(instance, method_name) + method(X, y, **kwargs) + + if metaestimator.get("preserves_metadata", True): + # sanity check that registry is not empty, or else the test + # passes trivially + assert registry + for estimator in registry: + check_recorded_metadata(estimator, method_name, **kwargs) + # clear the registry since the check could be different for the next + # method being tested + registry.clear() @pytest.mark.parametrize( @@ -268,10 +265,11 @@ def test_error_for_other_methods(metaestimator): X = metaestimator["X"] y = metaestimator["y"] routing_methods = metaestimator["routing_methods"] + warns_on = metaestimator["warns_on"] for method_name in routing_methods: - if metaestimator.get(method_name, {}).get("warns_on"): - # this method is not expected to warn + if method_name in warns_on: + # this method is expected to warn, not raise continue msg = ( From d7680f10d3299f7fdd03525ccca0ae2a785abf03 Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Wed, 10 Aug 2022 11:55:18 +0200 Subject: [PATCH 05/20] Address reviewer comment: recording None There is now an option for record_metadata to not store None values from kwargs. This is used in the tests now. --- sklearn/tests/test_metadata_routing.py | 16 +++++++---- .../test_metaestimators_metadata_routing.py | 28 +++++++++++++------ 2 files changed, 31 insertions(+), 13 deletions(-) diff --git a/sklearn/tests/test_metadata_routing.py b/sklearn/tests/test_metadata_routing.py index 77e7c20fd8ba7..c71da3e5e1e73 100644 --- a/sklearn/tests/test_metadata_routing.py +++ b/sklearn/tests/test_metadata_routing.py @@ -70,19 +70,25 @@ def assert_request_equal(request, dictionary): assert not len(getattr(request, method).requests) -def record_metadata(obj, method, **kwargs): - """Utility function to store passed metadata to a method.""" +def record_metadata(obj, method, record_none=True, **kwargs): + """Utility function to store passed metadata to a method. + + If record_none is False, kwargs whose values are None are skipped. This is + so that checks on keyword arguments whose default was not changed are + skipped. + + """ if not hasattr(obj, "_records"): obj._records = {} + if not record_none: + kwargs = {key: val for key, val in kwargs.items() if val is not None} obj._records[method] = kwargs def check_recorded_metadata(obj, method, **kwargs): """Check whether the expected metadata is passed to the object's method.""" records = getattr(obj, "_records", dict()).get(method, dict()) - # only check keys whose value was explicitly passed - expected_keys = {key for key, val in records.items() if val is not None} - assert set(kwargs.keys()) == expected_keys + assert set(kwargs.keys()) == set(records.keys()) for key, value in kwargs.items(): assert records[key] is value diff --git a/sklearn/tests/test_metaestimators_metadata_routing.py b/sklearn/tests/test_metaestimators_metadata_routing.py index eda1b71b5974e..a65831495f66c 100644 --- a/sklearn/tests/test_metaestimators_metadata_routing.py +++ b/sklearn/tests/test_metaestimators_metadata_routing.py @@ -1,5 +1,6 @@ import re import warnings +from functools import partial import numpy as np import pytest @@ -22,6 +23,9 @@ sample_weight = np.random.rand(N) +record_metadata_not_none = partial(record_metadata, record_none=False) + + class _Registry(list): # This list is used to get a reference to the sub-estimators, which are not # necessarily stored on the metaestimator. We need to override __deepcopy__ @@ -54,7 +58,7 @@ def partial_fit(self, X, y, sample_weight=None, metadata=None): if self.registry is not None: self.registry.append(self) - record_metadata( + record_metadata_not_none( self, "partial_fit", sample_weight=sample_weight, metadata=metadata ) return self @@ -63,14 +67,18 @@ def fit(self, X, y, sample_weight=None, metadata=None): if self.registry is not None: self.registry.append(self) - record_metadata(self, "fit", sample_weight=sample_weight, metadata=metadata) + record_metadata_not_none( + self, "fit", sample_weight=sample_weight, metadata=metadata + ) return self def predict(self, X, sample_weight=None, metadata=None): if self.registry is not None: self.registry.append(self) - record_metadata(self, "predict", sample_weight=sample_weight, metadata=metadata) + record_metadata_not_none( + self, "predict", sample_weight=sample_weight, metadata=metadata + ) return np.zeros(shape=(len(X),)) @@ -94,7 +102,7 @@ def partial_fit(self, X, y, sample_weight=None, metadata=None): if self.registry is not None: self.registry.append(self) - record_metadata( + record_metadata_not_none( self, "partial_fit", sample_weight=sample_weight, metadata=metadata ) self.classes_ = [0, 1] @@ -104,7 +112,9 @@ def fit(self, X, y, sample_weight=None, metadata=None): if self.registry is not None: self.registry.append(self) - record_metadata(self, "fit", sample_weight=sample_weight, metadata=metadata) + record_metadata_not_none( + self, "fit", sample_weight=sample_weight, metadata=metadata + ) self.classes_ = [0, 1] return self @@ -112,14 +122,16 @@ def predict(self, X, sample_weight=None, metadata=None): if self.registry is not None: self.registry.append(self) - record_metadata(self, "predict", sample_weight=sample_weight, metadata=metadata) + record_metadata_not_none( + self, "predict", sample_weight=sample_weight, metadata=metadata + ) return np.zeros(shape=(len(X),)) def predict_proba(self, X, sample_weight=None, metadata=None): if self.registry is not None: self.registry.append(self) - record_metadata( + record_metadata_not_none( self, "predict_proba", sample_weight=sample_weight, metadata=metadata ) return np.asarray([[0.0, 1.0]] * len(X)) @@ -128,7 +140,7 @@ def predict_log_proba(self, X, sample_weight=None, metadata=None): if self.registry is not None: self.registry.append(self) - record_metadata( + record_metadata_not_none( self, "predict_log_proba", sample_weight=sample_weight, metadata=metadata ) return np.zeros(shape=(len(X), 2)) From aceb4b45be0126200bd61491d36847a4a54b7d55 Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Wed, 10 Aug 2022 11:56:15 +0200 Subject: [PATCH 06/20] __copy__ has no arguments There was no error because it's not being used right now. --- sklearn/tests/test_metaestimators_metadata_routing.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sklearn/tests/test_metaestimators_metadata_routing.py b/sklearn/tests/test_metaestimators_metadata_routing.py index a65831495f66c..009bfb84bae22 100644 --- a/sklearn/tests/test_metaestimators_metadata_routing.py +++ b/sklearn/tests/test_metaestimators_metadata_routing.py @@ -34,7 +34,7 @@ class _Registry(list): def __deepcopy__(self, memo): return self - def __copy__(self, memo): + def __copy__(self): return self From d024a181cab06ad996b3c1645a6a0afc39879931 Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Wed, 10 Aug 2022 12:35:02 +0200 Subject: [PATCH 07/20] For expected errors, test each arg seperately Analogous to the warning test, we want to check each argument in isolation for the error case. --- .../test_metaestimators_metadata_routing.py | 42 ++++++++++++------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/sklearn/tests/test_metaestimators_metadata_routing.py b/sklearn/tests/test_metaestimators_metadata_routing.py index 009bfb84bae22..4a87b08d9b079 100644 --- a/sklearn/tests/test_metaestimators_metadata_routing.py +++ b/sklearn/tests/test_metaestimators_metadata_routing.py @@ -154,7 +154,10 @@ def predict_log_proba(self, X, sample_weight=None, metadata=None): "X": X, "y": y_multi, "routing_methods": ["fit", "partial_fit"], - "warns_on": {"fit": ["sample_weight"]}, + "warns_on": { + "fit": ["sample_weight", "metadata"], + "partial_fit": ["sample_weight"], + }, }, { "metaestimator": MultiOutputClassifier, @@ -163,7 +166,10 @@ def predict_log_proba(self, X, sample_weight=None, metadata=None): "X": X, "y": y_multi, "routing_methods": ["fit", "partial_fit"], - "warns_on": {"fit": ["sample_weight"]}, + "warns_on": { + "fit": ["sample_weight", "metadata"], + "partial_fit": ["sample_weight"], + }, }, { "metaestimator": CalibratedClassifierCV, @@ -186,9 +192,10 @@ def predict_log_proba(self, X, sample_weight=None, metadata=None): - X: X-data to fit and predict - y: y-data to fit - routing_methods: list of all methods to check for routing -- warns_on: A dict containing all methods as keys that are supposed to result in - a warning if routing is not requested. It is implied that all routing methods - not listed here should result in an error. +- warns_on: A dict containing all methods as keys, and arguments as values, + whose combination is supposed to result in a warning if routing is not + requested. It is implied that all routing methods and arguments not listed + here should result in an error. - preserves_metadata: Whether the metaestimator passes the metadata to the sub-estimator without modification or not. If it does, we check that the values are identical. If it doesn', no check is performed. TODO Maybe @@ -280,18 +287,21 @@ def test_error_for_other_methods(metaestimator): warns_on = metaestimator["warns_on"] for method_name in routing_methods: - if method_name in warns_on: - # this method is expected to warn, not raise - continue + warn_args = warns_on.get(method_name, []) + for key in ["sample_weight", "metadata"]: + if key in warn_args: + # this method is expected to warn for this argument, not raise + continue - msg = ( - "[sample_weight, metadata] are passed but are not explicitly set as " - "requested or not for " - f"{estimator.__class__.__name__}.{method_name}" - ) - with pytest.raises(UnsetMetadataPassedError, match=re.escape(msg)): - method = getattr(instance, method_name) - method(X, y, sample_weight=sample_weight, metadata=metadata) + val = {"sample_weight": sample_weight, "metadata": metadata}[key] + kwargs = {key: val} + msg = ( + f"[{key}] are passed but are not explicitly set as requested or not for" + f" {estimator.__class__.__name__}.{method_name}" + ) + with pytest.raises(UnsetMetadataPassedError, match=re.escape(msg)): + method = getattr(instance, method_name) + method(X, y, **kwargs) @pytest.mark.parametrize( From f9976eebdaec2803bf8ff3cac48eecd2e1f7a9b5 Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Wed, 10 Aug 2022 12:44:49 +0200 Subject: [PATCH 08/20] Fit the metaestimator if not done by method The could be methods like "score" that require a fitted metaestimator. Therefore, it is fitted before calling the tested method, except of the tested method is a fitting method. Note that right now, this never happens, so in a way that code path is untested. --- sklearn/tests/test_metaestimators_metadata_routing.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/sklearn/tests/test_metaestimators_metadata_routing.py b/sklearn/tests/test_metaestimators_metadata_routing.py index 4a87b08d9b079..d04d07c615fe1 100644 --- a/sklearn/tests/test_metaestimators_metadata_routing.py +++ b/sklearn/tests/test_metaestimators_metadata_routing.py @@ -234,7 +234,6 @@ def test_warning_for_indicated_methods(metaestimator): registry = _Registry() estimator = metaestimator["estimator"](registry=registry) estimator_name = metaestimator["estimator_name"] - instance = cls(**{estimator_name: estimator}) X = metaestimator["X"] y = metaestimator["y"] routing_methods = metaestimator["routing_methods"] @@ -254,6 +253,10 @@ def test_warning_for_indicated_methods(metaestimator): f" following error: [{key}] are passed but are not explicitly set as" f" requested or not for {estimator.__class__.__name__}.{method_name}" ) + + instance = cls(**{estimator_name: estimator}) + if "fit" not in method_name: # instance needs to be fitted first + instance.fit(X, y) with pytest.warns(FutureWarning, match=re.escape(warn_msg)): method = getattr(instance, method_name) method(X, y, **kwargs) @@ -280,7 +283,6 @@ def test_error_for_other_methods(metaestimator): cls = metaestimator["metaestimator"] estimator = metaestimator["estimator"]() estimator_name = metaestimator["estimator_name"] - instance = cls(**{estimator_name: estimator}) X = metaestimator["X"] y = metaestimator["y"] routing_methods = metaestimator["routing_methods"] @@ -299,6 +301,10 @@ def test_error_for_other_methods(metaestimator): f"[{key}] are passed but are not explicitly set as requested or not for" f" {estimator.__class__.__name__}.{method_name}" ) + + instance = cls(**{estimator_name: estimator}) + if "fit" not in method_name: # instance needs to be fitted first + instance.fit(X, y) with pytest.raises(UnsetMetadataPassedError, match=re.escape(msg)): method = getattr(instance, method_name) method(X, y, **kwargs) From a3536712881c86fab7a010f5d3bcedfc46af92b4 Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Mon, 15 Aug 2022 12:22:24 +0200 Subject: [PATCH 09/20] Ignore type checking on CheckingClassifier Specifically, overriding set_fit_request. --- sklearn/utils/_mocking.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sklearn/utils/_mocking.py b/sklearn/utils/_mocking.py index cc73e407a89b0..081224fe7d980 100644 --- a/sklearn/utils/_mocking.py +++ b/sklearn/utils/_mocking.py @@ -325,7 +325,7 @@ def _more_tags(self): # Deactivate key validation for CheckingClassifier because we want to be able to # call fit with arbitrary fit_params and record them. Without this change, we # would get an error because those arbitrary params are not expected. -CheckingClassifier.set_fit_request = RequestMethod( # noqa +CheckingClassifier.set_fit_request = RequestMethod( # type: ignore name="fit", keys=[], validate_keys=False ) From d75b36880c6c3ab2c7772a739c14291125d2ddb8 Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Mon, 15 Aug 2022 12:23:41 +0200 Subject: [PATCH 10/20] Add routing of groups to splitter --- sklearn/calibration.py | 6 +++++- sklearn/tests/test_calibration.py | 19 ++++++++++++++++++- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/sklearn/calibration.py b/sklearn/calibration.py index 990ac3ed5a06d..8ced38c9d5219 100644 --- a/sklearn/calibration.py +++ b/sklearn/calibration.py @@ -381,7 +381,7 @@ def fit(self, X, y, sample_weight=None, **fit_params): sample_weight=sample_weight, fit_params=routed_params.estimator.fit, ) - for train, test in cv.split(X, y) + for train, test in cv.split(X, y, **routed_params.splitter.split) ) else: this_estimator = clone(estimator) @@ -486,6 +486,10 @@ def get_metadata_routing(self): estimator=self._get_estimator(), method_mapping=MethodMapping().add(callee="fit", caller="fit"), ) + .add( + splitter=self.cv, + method_mapping=MethodMapping().add(callee="split", caller="fit") + ) # the fit method already accepts everything, therefore we don't # specify parameters. The value passed to ``child`` needs to be the # same as what's passed to ``add`` above, in this case diff --git a/sklearn/tests/test_calibration.py b/sklearn/tests/test_calibration.py index a02735fbe2d76..3fd333aeb4bdc 100644 --- a/sklearn/tests/test_calibration.py +++ b/sklearn/tests/test_calibration.py @@ -19,7 +19,7 @@ from sklearn.exceptions import NotFittedError from sklearn.datasets import make_classification, make_blobs, load_iris from sklearn.preprocessing import LabelEncoder -from sklearn.model_selection import KFold, cross_val_predict +from sklearn.model_selection import GroupKFold, KFold, cross_val_predict from sklearn.naive_bayes import MultinomialNB from sklearn.ensemble import ( RandomForestClassifier, @@ -1063,3 +1063,20 @@ def test_calibrated_classifier_deprecation_base_estimator(data): warn_msg = "`base_estimator` was renamed to `estimator`" with pytest.warns(FutureWarning, match=warn_msg): calibrated_classifier.fit(*data) + + +def test_calibration_groupkfold(data): + # Check that groups are routed to the splitter + X, y = data + split_groups = np.array([0, 1] * (len(y) // 2)) + + class MyGroupKFold(GroupKFold): + """Custom Splitter that checks that the values of groups are correct""" + def split(self, X, y=None, groups=None): + assert (groups == split_groups).all() + return super().split(X, y=y, groups=groups) + + cv = MyGroupKFold(n_splits=2) + calib_clf = CalibratedClassifierCV(cv=cv) + # check that fitting does not raise an error + calib_clf.fit(X, y, groups=split_groups) From c3a1d67df287569f0335bba9661c03d0fb77fc9c Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Mon, 15 Aug 2022 12:29:48 +0200 Subject: [PATCH 11/20] Black formatting --- sklearn/calibration.py | 2 +- sklearn/tests/test_calibration.py | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/sklearn/calibration.py b/sklearn/calibration.py index 8ced38c9d5219..43dbf93c1656a 100644 --- a/sklearn/calibration.py +++ b/sklearn/calibration.py @@ -488,7 +488,7 @@ def get_metadata_routing(self): ) .add( splitter=self.cv, - method_mapping=MethodMapping().add(callee="split", caller="fit") + method_mapping=MethodMapping().add(callee="split", caller="fit"), ) # the fit method already accepts everything, therefore we don't # specify parameters. The value passed to ``child`` needs to be the diff --git a/sklearn/tests/test_calibration.py b/sklearn/tests/test_calibration.py index 3fd333aeb4bdc..7abbec4b1f008 100644 --- a/sklearn/tests/test_calibration.py +++ b/sklearn/tests/test_calibration.py @@ -1072,6 +1072,7 @@ def test_calibration_groupkfold(data): class MyGroupKFold(GroupKFold): """Custom Splitter that checks that the values of groups are correct""" + def split(self, X, y=None, groups=None): assert (groups == split_groups).all() return super().split(X, y=y, groups=groups) From ce3dabd46a3f9bd7c08067004b79a91bb4292f2b Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Tue, 16 Aug 2022 16:14:25 +0200 Subject: [PATCH 12/20] Fix typo --- sklearn/tests/test_metadata_routing.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sklearn/tests/test_metadata_routing.py b/sklearn/tests/test_metadata_routing.py index c71da3e5e1e73..baf309f0d5348 100644 --- a/sklearn/tests/test_metadata_routing.py +++ b/sklearn/tests/test_metadata_routing.py @@ -39,7 +39,7 @@ def assert_request_is_empty(metadata_request, exclude=None): """Check if a metadata request dict is empty. One can exclude a method or a list of methods from the check using the - ``exclude`` perameter. + ``exclude`` parameter. """ if isinstance(metadata_request, MetadataRouter): for _, route_mapping in metadata_request: From ad1b06276e9d1ad116059c3bca24a3b7e29c2ea4 Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Tue, 16 Aug 2022 16:16:24 +0200 Subject: [PATCH 13/20] Reviewer request: Change checking of routed data Give option to not check if the routed metadata are the literal string "default" (instead of checking for None). --- sklearn/tests/test_metadata_routing.py | 16 +++++---- .../test_metaestimators_metadata_routing.py | 34 +++++++++---------- 2 files changed, 27 insertions(+), 23 deletions(-) diff --git a/sklearn/tests/test_metadata_routing.py b/sklearn/tests/test_metadata_routing.py index baf309f0d5348..51c8645e42e5c 100644 --- a/sklearn/tests/test_metadata_routing.py +++ b/sklearn/tests/test_metadata_routing.py @@ -70,18 +70,22 @@ def assert_request_equal(request, dictionary): assert not len(getattr(request, method).requests) -def record_metadata(obj, method, record_none=True, **kwargs): +def record_metadata(obj, method, record_default=True, **kwargs): """Utility function to store passed metadata to a method. - If record_none is False, kwargs whose values are None are skipped. This is - so that checks on keyword arguments whose default was not changed are - skipped. + If record_default is False, kwargs whose values are "default" are skipped. + This is so that checks on keyword arguments whose default was not changed + are skipped. """ if not hasattr(obj, "_records"): obj._records = {} - if not record_none: - kwargs = {key: val for key, val in kwargs.items() if val is not None} + if not record_default: + kwargs = { + key: val + for key, val in kwargs.items() + if not isinstance(val, str) or (val != "default") + } obj._records[method] = kwargs diff --git a/sklearn/tests/test_metaestimators_metadata_routing.py b/sklearn/tests/test_metaestimators_metadata_routing.py index d04d07c615fe1..603e3ae1e8d99 100644 --- a/sklearn/tests/test_metaestimators_metadata_routing.py +++ b/sklearn/tests/test_metaestimators_metadata_routing.py @@ -23,7 +23,7 @@ sample_weight = np.random.rand(N) -record_metadata_not_none = partial(record_metadata, record_none=False) +record_metadata_not_default = partial(record_metadata, record_default=False) class _Registry(list): @@ -54,29 +54,29 @@ class ConsumingRegressor(RegressorMixin, BaseEstimator): def __init__(self, registry=None): self.registry = registry - def partial_fit(self, X, y, sample_weight=None, metadata=None): + def partial_fit(self, X, y, sample_weight="default", metadata="default"): if self.registry is not None: self.registry.append(self) - record_metadata_not_none( + record_metadata_not_default( self, "partial_fit", sample_weight=sample_weight, metadata=metadata ) return self - def fit(self, X, y, sample_weight=None, metadata=None): + def fit(self, X, y, sample_weight="default", metadata="default"): if self.registry is not None: self.registry.append(self) - record_metadata_not_none( + record_metadata_not_default( self, "fit", sample_weight=sample_weight, metadata=metadata ) return self - def predict(self, X, sample_weight=None, metadata=None): + def predict(self, X, sample_weight="default", metadata="default"): if self.registry is not None: self.registry.append(self) - record_metadata_not_none( + record_metadata_not_default( self, "predict", sample_weight=sample_weight, metadata=metadata ) return np.zeros(shape=(len(X),)) @@ -98,49 +98,49 @@ class ConsumingClassifier(ClassifierMixin, BaseEstimator): def __init__(self, registry=None): self.registry = registry - def partial_fit(self, X, y, sample_weight=None, metadata=None): + def partial_fit(self, X, y, sample_weight="default", metadata="default"): if self.registry is not None: self.registry.append(self) - record_metadata_not_none( + record_metadata_not_default( self, "partial_fit", sample_weight=sample_weight, metadata=metadata ) self.classes_ = [0, 1] return self - def fit(self, X, y, sample_weight=None, metadata=None): + def fit(self, X, y, sample_weight="default", metadata="default"): if self.registry is not None: self.registry.append(self) - record_metadata_not_none( + record_metadata_not_default( self, "fit", sample_weight=sample_weight, metadata=metadata ) self.classes_ = [0, 1] return self - def predict(self, X, sample_weight=None, metadata=None): + def predict(self, X, sample_weight="default", metadata="default"): if self.registry is not None: self.registry.append(self) - record_metadata_not_none( + record_metadata_not_default( self, "predict", sample_weight=sample_weight, metadata=metadata ) return np.zeros(shape=(len(X),)) - def predict_proba(self, X, sample_weight=None, metadata=None): + def predict_proba(self, X, sample_weight="default", metadata="default"): if self.registry is not None: self.registry.append(self) - record_metadata_not_none( + record_metadata_not_default( self, "predict_proba", sample_weight=sample_weight, metadata=metadata ) return np.asarray([[0.0, 1.0]] * len(X)) - def predict_log_proba(self, X, sample_weight=None, metadata=None): + def predict_log_proba(self, X, sample_weight="default", metadata="default"): if self.registry is not None: self.registry.append(self) - record_metadata_not_none( + record_metadata_not_default( self, "predict_log_proba", sample_weight=sample_weight, metadata=metadata ) return np.zeros(shape=(len(X), 2)) From fdd7a894a3c2c9ef072c888069800c28fe6cd97e Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Tue, 16 Aug 2022 16:27:46 +0200 Subject: [PATCH 14/20] Empty-Commit From b8d459d7455e68a8b2659dcd11f2d574a5875a96 Mon Sep 17 00:00:00 2001 From: "Thomas J. Fan" Date: Tue, 16 Aug 2022 11:02:33 -0400 Subject: [PATCH 15/20] CI empty-commit From 62867d9d57ecdbc5ce28253f4f51ab31e474a31f Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Tue, 16 Aug 2022 17:15:01 +0200 Subject: [PATCH 16/20] Yet another empty commit to try if CI works From 57b292b105a36859e6bb3e0a49587e52d91483bb Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Wed, 17 Aug 2022 10:03:45 +0200 Subject: [PATCH 17/20] New empty commit to try if CI works now From 98f99042e188c3bf059cadd512b19c752caac8df Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Wed, 17 Aug 2022 10:19:32 +0200 Subject: [PATCH 18/20] Empty commit after creating CircleCI acct From 0a54eb8297b358f75e440c2b8cc5444882dbce25 Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Wed, 17 Aug 2022 15:23:24 +0200 Subject: [PATCH 19/20] Don't explicitly check groups values in test This is already covererd by the general routing tests. --- sklearn/tests/test_calibration.py | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/sklearn/tests/test_calibration.py b/sklearn/tests/test_calibration.py index 7abbec4b1f008..8ab31df791c61 100644 --- a/sklearn/tests/test_calibration.py +++ b/sklearn/tests/test_calibration.py @@ -1068,16 +1068,8 @@ def test_calibrated_classifier_deprecation_base_estimator(data): def test_calibration_groupkfold(data): # Check that groups are routed to the splitter X, y = data - split_groups = np.array([0, 1] * (len(y) // 2)) - - class MyGroupKFold(GroupKFold): - """Custom Splitter that checks that the values of groups are correct""" - - def split(self, X, y=None, groups=None): - assert (groups == split_groups).all() - return super().split(X, y=y, groups=groups) - - cv = MyGroupKFold(n_splits=2) + groups = np.array([0, 1] * (len(y) // 2)) # assumes len(y) is even + cv = GroupKFold(n_splits=2) calib_clf = CalibratedClassifierCV(cv=cv) # check that fitting does not raise an error - calib_clf.fit(X, y, groups=split_groups) + calib_clf.fit(X, y, groups=groups) From 8ae3475075b6cc0e4a500482794c4a6105eb68bf Mon Sep 17 00:00:00 2001 From: adrinjalali Date: Wed, 17 Aug 2022 16:29:04 +0200 Subject: [PATCH 20/20] trigger ci