FEAT add SLEP006 with a feature flag by adrinjalali · Pull Request #26103 · scikit-learn/scikit-learn

adrinjalali · 2023-04-05T15:34:13Z

This PR adds a enable_metadata_routing flag as a global configuration, which is False by default.

A good way to review this PR is to compare some of the files with main instead of sample-props.

test_calibration.py and test_multioutput.py are copied from main here, so the diff here is only compared to sample-props branch, and this PR roles back previous changes to these files.

towards: #26045

adrinjalali · 2023-04-05T15:38:21Z

sklearn/calibration.py

-            # 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)


we don't need to be backward compatible now when the feature flag is on, so these lines are removed.

adrinjalali · 2023-04-05T15:47:40Z

sklearn/tests/test_metaestimators_metadata_routing.py

-        "warns_on": {
-            "fit": ["sample_weight", "metadata"],
-            "partial_fit": ["sample_weight"],
-        },


removing these and corresponding tests since we don't deal with backward compatibility now.

glemaitre

The changes looks good. In a subsequent PR, I think that we should have a way to test both legacy weighting and the new metadata routing at least in the common test.

doc/metadata_routing.rst

glemaitre · 2023-04-13T12:46:49Z

sklearn/utils/estimator_checks.py

            return re.sub(r"\s", "", str(obj))


-def _weighted(estimator):


I am wondering if a subsequent PR, we could have a way to test the legacy and new way:

SKLEARN_METADATA_ROUTING="enable" pytest sklearn/test/test_common.py

and then have two constructor selected depending on the config.

Common test could be quite useful to catch up some bugs.

yeah there can be some tests in common tests, happy to work on it on a subsequent PR (but probably not a blocker to merge sample-props into main).

glemaitre · 2023-04-13T12:49:29Z

sklearn/multioutput.py

@@ -121,15 +126,26 @@ def partial_fit(self, X, y, classes=None, sample_weight=None, **partial_fit_para
            weights.

        **partial_fit_params : dict of str -> object


For reviewer: This parameter was added in sample_props branch.

sklearn/multioutput.py

glemaitre · 2023-04-13T13:11:44Z

sklearn/metrics/_scorer.py

+            )
+
+        if sample_weight is not None:
+            kwargs["sample_weight"] = sample_weight


I was about to say that we modify a mutable here and it should be safer to copy. However, now I see that we pass **kwargs and not kwargs so it should be fine.

sklearn/metrics/_scorer.py

glemaitre · 2023-04-13T13:17:52Z

sklearn/tests/test_multioutput.py

-    SGDRegressor,
-    QuantileRegressor,
-)
+from sklearn.linear_model import Lasso


I would like so much to have isort to avoid thinking about those changes :)

adrinjalali

Right now we basically test all the old routing in our tests, and we have dedicated tests for the new routing. But we can certainly work on some tests for common tests, and have a list of meta estimators for which they fail.

adrinjalali · 2023-04-13T14:16:50Z

sklearn/multioutput.py

-        routed_params = process_routing(
-            obj=self, method="fit", other_params=fit_params, sample_weight=sample_weight
-        )
+        if _routing_enabled():


maybe worth a PR then :P

adrinjalali · 2023-04-13T14:22:50Z

sklearn/utils/estimator_checks.py

            return re.sub(r"\s", "", str(obj))


-def _weighted(estimator):


yeah there can be some tests in common tests, happy to work on it on a subsequent PR (but probably not a blocker to merge sample-props into main).

glemaitre

LGTM. Thanks @adrinjalali

ogrisel

I think we should completely get rid of .warn_on. The warning message would not make sense for third party library and we won't need it in scikit-learn.

Besides that, looks good to me. But I would need to see the PR against main to decide whether this is mergeable in main as it is or not.

sklearn/tests/test_metaestimators_metadata_routing.py

sklearn/tests/test_calibration.py

sklearn/tests/test_metaestimators_metadata_routing.py

ogrisel · 2023-04-14T19:03:27Z

sklearn/multioutput.py


 class _MultiOutputEstimator(MetaEstimatorMixin, BaseEstimator, metaclass=ABCMeta):
-    _parameter_constraints = {
+    _parameter_constraints: dict = {


seems unrelated and unusual in scikit-learn.

I'd agree, but this one is also not me here, it's from main:

scikit-learn/sklearn/multioutput.py

Line 87 in 66a4d96

_parameter_constraints: dict = {

mypy stuff

doc/metadata_routing.rst

adrinjalali

Besides that, looks good to me. But I would need to see the PR against main to decide whether this is mergeable in main as it is or not.

@ogrisel it isn't ready for merge after this. I'll submit another PR once this is merged to prepare the sample-props branch for merge, with all the nits we need to do before merging, after that PR we can review the big PR and merge.

doc/metadata_routing.rst

adrinjalali · 2023-04-15T10:32:34Z

sklearn/multioutput.py


 class _MultiOutputEstimator(MetaEstimatorMixin, BaseEstimator, metaclass=ABCMeta):
-    _parameter_constraints = {
+    _parameter_constraints: dict = {


I'd agree, but this one is also not me here, it's from main:

scikit-learn/sklearn/multioutput.py

Line 87 in 66a4d96

_parameter_constraints: dict = {

glemaitre · 2023-04-17T10:00:52Z

Merging since all points had been addressed and the remaining discussion will be linked to another PR.

adrinjalali added 6 commits March 14, 2023 15:21

MNT remove backward compatibility from meta-estimators

5fb1f50

Merge branch 'slep6/non-bc' into slep6/feature-flag

82589be

FEAT add SLEP006 as a feature enabled by a feature flag

ee03818

use old calibration tests

8c818b7

use old test_multioutput.py

2f6abcb

set config in sklearn/tests/test_metaestimators_metadata_routing.py

f589736

github-actions bot added module:metrics module:utils labels Apr 5, 2023

adrinjalali changed the title ~~MNT remove backward compatibility from meta-estimators~~ FEAT add SLEP006 with a feature flag Apr 5, 2023

add back scorer backward compatibility

2396f31

adrinjalali commented Apr 5, 2023

View reviewed changes

adrinjalali added the No Changelog Needed label Apr 5, 2023

adrinjalali mentioned this pull request Apr 5, 2023

MNT SLEP006: Remove backward compatibility from meta-estimators #25852

Closed

adrinjalali added 2 commits April 6, 2023 11:24

fix scorers and reduce diff with main

61845f2

scorer minor fix

fb0fdf2

adrinjalali marked this pull request as ready for review April 6, 2023 12:34

adrinjalali mentioned this pull request Apr 6, 2023

SLEP006 - Metadata Routing task list #22893

Open

28 tasks

glemaitre self-requested a review April 6, 2023 15:14

glemaitre reviewed Apr 13, 2023

View reviewed changes

adrinjalali commented Apr 13, 2023

View reviewed changes

apply Guillaume's suggestions

b7fb316

glemaitre approved these changes Apr 14, 2023

View reviewed changes

ogrisel approved these changes Apr 14, 2023

View reviewed changes

adrinjalali commented Apr 15, 2023

View reviewed changes

remove warn_on, and other suggestions from Olivier

54deade

glemaitre merged commit d2cb8d5 into scikit-learn:sample-props Apr 17, 2023

adrinjalali deleted the slep6/feature-flag branch April 17, 2023 10:00

This was referenced Jun 3, 2023

SLEP006 - Add Metadata Routing to LogisticRegressionCV #24498

Closed

SLEP006: Metadata routing for bagging #24250

Closed

haiatn mentioned this pull request Jul 29, 2023

SLEP006: introduction of metadata routing through a feature flag #26045

Open

		@@ -121,15 +126,26 @@ def partial_fit(self, X, y, classes=None, sample_weight=None, **partial_fit_para
		weights.

		**partial_fit_params : dict of str -> object

Uh oh!

Conversation

adrinjalali commented Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glemaitre commented Apr 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adrinjalali commented Apr 5, 2023 •

edited

Loading