✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

_{Generated for commit: b118ffe. Link to the linter CI: here}

BaggingRegressor(estimator=Ridge()) now passes the statistical test for sample weight equivalence with repeated data.

But BaggingClassifier(estimator=LogisticRegression()) doesn't :(

Any idea @snath-xoc on why it fails on the classifier ?

I think the statistical sample weight equivalence is only guaranteed when samples are drawn with replacement (bootstrap=True). We should maybe warn the user if they use sample_weight with bootstrap=False ?

I might need your help @adrinjalali @StefanieSenger for fixing the metadata test failure 🙏

test_error_on_missing_requests_for_sub_estimator[BaggingRegressor]
Failed: DID NOT RAISE <class 'sklearn.exceptions.UnsetMetadataPassedError'>

Now BaggingRegressor.fit is supposed to consumes sample_weight for drawing samples and not route/pass sample_weight to underlying estimators.

Is the test adapted for this case ? If not should we write a test for the new intended behavior ?

Hi @antoinebaker, strange that logisticregression is giving such an output, is it still the case? If so I can take a quick look.

Hi @antoinebaker, strange that logisticregression is giving such an output, is it still the case? If so I can take a quick look.

Ah, good news, I think I found the reason 😃

The culprit is max_samples, which by default is max_samples=1.0 (meaning taking n_samples).
For the repeated/weighted estimators to be equivalent we need to draw

1. the same number of samples
2. with replacement

Condition 2 requires using bootstrap=True
Condition 1 requires to use an integer max_samples. Using a float max_samples (relative size) will lead to different number of samples between weighted/repeated.

Now all these estimators pass the statistical test:

# max_samples: int (here max_samples ~ n_samples)
BaggingRegressor(estimator=Ridge(), max_samples=100)
BaggingClassifier(estimator=LogisticRegression(), max_samples=100)
# subsampling (max_samples ~ 0.1 n_samples) and n_estimators=100
BaggingRegressor(estimator=Ridge(), max_samples=10, n_estimators=100)
BaggingClassifier(estimator=LogisticRegression(), max_samples=10, n_estimators=100)

And these fail as they should:

# bootstrap=False (drawing without replacement)
BaggingRegressor(estimator=Ridge(), bootstrap=False, max_samples=10, n_estimators=100)
BaggingClassifier(estimator=LogisticRegression(), bootstrap=False, max_samples=10, n_estimators=100)

To illustrate, BaggingRegressor(estimator=Ridge(), max_samples=100)

BaggingClassifier(estimator=LogisticRegression(), max_samples=100)

Apart from the test failures #31165 (comment) that require metadata experts, I think this is ready for review.

@StefanieSenger 's analysis is mostly correct. The test expects all metadata to be always routed if a method is declared as "routing", and now with this PR it's no longer true.

I propose the following patch to fix / improve the tests:

diff --git a/sklearn/tests/test_metaestimators_metadata_routing.py b/sklearn/tests/test_metaestimators_metadata_routing.py
index ae2a186a3c..6a84d3b8dd 100644
--- a/sklearn/tests/test_metaestimators_metadata_routing.py
+++ b/sklearn/tests/test_metaestimators_metadata_routing.py
@@ -330,7 +330,7 @@ METAESTIMATORS: list = [
         "y": y,
         "preserves_metadata": False,
         "estimator_routing_methods": [
-            "fit",
+            ("fit", ["metadata"]),
             "predict",
             "predict_proba",
             "predict_log_proba",
@@ -349,7 +349,7 @@ METAESTIMATORS: list = [
         "X": X,
         "y": y,
         "preserves_metadata": False,
-        "estimator_routing_methods": ["fit", "predict"],
+        "estimator_routing_methods": [("fit", ["metadata"]), "predict"],
     },
     {
         "metaestimator": RidgeCV,
@@ -459,7 +459,13 @@ The keys are as follows:
 - X: X-data to fit and predict
 - y: y-data to fit
 - estimator_routing_methods: list of all methods to check for routing metadata
-  to the sub-estimator
+  to the sub-estimator. Each value is either a str or a tuple:
+    - str: the name of the method, all metadata in this method must be routed to the
+      sub-estimator
+    - tuple: the name of the method, the second element is a list of metadata keys
+      to be passed to the sub-estimator. This is useful if certain metadata such as
+      `sample_weight` are never routed and only consumed, such as in `BaggingClassifier`
+      and `BaggingRegressor`.
 - preserves_metadata:
     - True (default): the metaestimator passes the metadata to the
       sub-estimator without modification. We check that the values recorded by
@@ -561,6 +567,22 @@ def get_init_args(metaestimator_info, sub_estimator_consumes):
         (cv, cv_registry),
     )
 
+def process_routing_methods(estimator_routing_methods):
+    """Process estimator_routing_methods and return a dict.
+
+    The dictionary is of the form {"method": ["metadata", ...]}.
+    By default, the list includes `sample_weight` and `metadata`.
+    """
+    res = dict()
+    for method_spec in estimator_routing_methods:
+        if isinstance(method_spec, str):
+            method = method_spec
+            metadata = ["sample_weight", "metadata"]
+        else:
+            method, metadata = method_spec
+        res[method] = metadata
+    return res
+
 
 def set_requests(obj, *, method_mapping, methods, metadata_name, value=True):
     """Call `set_{method}_request` on a list of methods from the sub-estimator.
@@ -662,10 +684,10 @@ def test_error_on_missing_requests_for_sub_estimator(metaestimator):
     metaestimator_class = metaestimator["metaestimator"]
     X = metaestimator["X"]
     y = metaestimator["y"]
-    routing_methods = metaestimator["estimator_routing_methods"]
+    routing_methods = process_routing_methods(metaestimator["estimator_routing_methods"])
 
-    for method_name in routing_methods:
-        for key in ["sample_weight", "metadata"]:
+    for method_name, metadata_keys in routing_methods.items():
+        for key in metadata_keys:
             kwargs, (estimator, _), (scorer, _), *_ = get_init_args(
                 metaestimator, sub_estimator_consumes=True
             )
@@ -721,12 +743,12 @@ def test_setting_request_on_sub_estimator_removes_error(metaestimator):
     metaestimator_class = metaestimator["metaestimator"]
     X = metaestimator["X"]
     y = metaestimator["y"]
-    routing_methods = metaestimator["estimator_routing_methods"]
+    routing_methods =   process_routing_methods(metaestimator["estimator_routing_methods"])
     method_mapping = metaestimator.get("method_mapping", {})
     preserves_metadata = metaestimator.get("preserves_metadata", True)
 
-    for method_name in routing_methods:
-        for key in ["sample_weight", "metadata"]:
+    for method_name, metadata_keys in routing_methods.items():
+        for key in metadata_keys:
             val = {"sample_weight": sample_weight, "metadata": metadata}[key]
             method_kwargs = {key: val}
 
@@ -797,7 +819,7 @@ def test_non_consuming_estimator_works(metaestimator):
     metaestimator_class = metaestimator["metaestimator"]
     X = metaestimator["X"]
     y = metaestimator["y"]
-    routing_methods = metaestimator["estimator_routing_methods"]
+    routing_methods = process_routing_methods(metaestimator["estimator_routing_methods"])
 
     for method_name in routing_methods:
         kwargs, (estimator, _), (_, _), (_, _) = get_init_args(

I think the statistical sample weight equivalence is only guaranteed when samples are drawn with replacement (bootstrap=True). We should maybe warn the user if they use sample_weight with bootstrap=False ?

I think we should at least document this in the docstring entries for the bootstrap parameter in __init__ and in the docstring for sample_weight in the fit method.

We could also raise a warning when bootstrap=False and sample_weight is not None.

I wonder if we shouldn't keep the code of main as an option, since it is typically 2x more memory efficient than the new code.

Maybe we could introduce a new hyper-parameter resampling that takes values in {"concrete", "using_sample_weight", "auto"}.

• "concrete" would use the new code of this PR;
• "using_sample_weight" would use the old code (bincount of the sampled indices);
• "auto" (the default) would:
  • branch to "concrete" if the Bagging model is itself provided non-None sample_weight or if the base estimator does not accept sample_weight.
  • branch to "using_sample_weight" otherwise, for the sake of keeping memory efficient code by default.

Unfortunately, it would be expensive to test that BaggingClassifier(resampling="using_sample_weight").fit(X, y, sample_weight=None) and BaggingClassifier(resampling="concrete", sample_weight=None).fit(X, y) are identically distributed.

But we could at least check that they do qualitatively as part of the review process of this PR using the following plot:

import matplotlib.pyplot as plt
import sklearn

fig, ax = plt.subplots()
shared = dict(
    param_name="n_estimators",
    param_range=[1, 5, 10, 50],
    scoring="neg_log_loss",
    cv=sklearn.model_selection.ShuffleSplit(n_splits=10),
    fit_params={"sample_weight": None},  # to be explicit that we would not expect the curves to match otherwise
    ax=ax,
)
sklearn.inspection.ValidationCurveDisplay.from_estimator(
    sklearn.ensemble.BaggingClassifier(resampling="using_sample_weight"), X, y, **shared
)

sklearn.inspection.ValidationCurveDisplay.from_estimator(
    sklearn.ensemble.BaggingClassifier(resampling="concrete"), X, y, **shared
)
plt.legend()

and check that the 2 sets of curves (train and test for each estimator variant) and error bars overlap. If not, increase n_splits and retry to check that any discrepancy disappears in the limits of large n_splits values.

I wonder if we shouldn't keep the code of main as an option, since it is typically 2x more memory efficient than the new code.

@ogrisel Could you elaborate why the code of main is more memory efficient ? Along the same lines I did not grasp this comment in main:

In my understanding, the current sampling with weights, when max_samples < n_samples, will pass smaller X_ y_ arrays to the subestimators and should be more compute efficient. I guess _safe_indexing merely creates views, so it should not impact the memory ? I feel I am missing something ;)

Thanks @StefanieSenger for the detailed analysis and @adrinjalali for the patch, it did fix the metadata routing test :)

In my understanding, the current sampling with weights, when max_samples < n_samples, will pass smaller X_ y_ arrays to the subestimators and should be more compute efficient.

I agree that when max_samples << n_samples, using indexing is more time efficient than using weighting (and the memory overhead is limited if max_samples is small enough).

I guess _safe_indexing merely creates views, so it should not impact the memory ?

No, it's not possible to create views with fancy (integer) indexing: NumPy is forced to allocate memory to store the result of the indexing operation (contrary to slicing):

>>> import numpy as np
>>> a = np.random.randn(12, 3)
>>> idx = np.asarray([3, 5, 3, 0])
>>> b = a.take(idx, axis=0)  # integer indexing does not create a view
>>> np.may_share_memory(a, b)
False
>>> c = a[3:5]  # slicing creates a view
>>> np.may_share_memory(a, c)
True

No, it's not possible to create views with fancy (integer) indexing: NumPy is forced to allocate memory to store the result of the indexing operation (contrary to slicing)

Ah, thanks for the explanation !