Simplify sample_weights handling (internal refactoring)

[Valentin-Laurent]

Release v1 has simplified the way sample_weights are passed as arguments (directly in fit_params).

We can simplify the fit methods of classes _MapieRegressor, _MapieQuantileRegressor and _MapieClassifier to include sample_weight directly in fit_params as well, thus removing the need of the specific _prepare_fit_params_and_sample_weight function and its usage in ConformalizedQuantileRegressor, CrossConformalRegressor, JackknifeAfterBootstrapRegressor and CrossConformalClassifier.

We can also most probably remove _fit_estimator (even for calibration), and replace its usage by a simple estimator.fit(X, y, **fit_params) (I believe the checks performed in this function are too specific and unnecessary once sample_weights are included in fit_params

[VishnoiAman777]

Hi @Valentin-Laurent,

I'd like to contribute to this issue, and I have a couple of questions regarding the codebase refactoring.

Even if we pass the fit parameters directly to classes like _MapieRegressor, there are specific checks (e.g., in init_fit and _check_fit_parameters) that have a check for sample_weight to be non-null. This means we would need to pop sample_weight from fit_params to perform these checks, which essentially duplicates the logic from _prepare_fit_params_and_sample_weight—except for the deepcopy performed in _prepare_params function.

Is this the intended approach, or is there a preferred way you’d like these checks and parameter preparations to be handled?

[Valentin-Laurent]

Hello @VishnoiAman777 , thank you for your intent to contribute.
It seems that during my investigation I missed the checks you're mentioning (for eg _check_null_weight).

So I guess the simplification is not that obvious.

Regarding _check_null_weight for example, in v1 we're not using it anymore in split conformal, and this seems to be fine (when developing v1, we had non-regression tests to make sure we got the same results before and after v1). But we're still using it in cross conformal. To be honest I'm unsure if this is necessary, but my point is that simplifying the handling of sample weights is not as straightforward as I expected.

Let me know what you think, but I see roughly three direction you can take here:

• Either you're willing to tackle this entire topic, and in that case we would need to understand every operation performed on sample_weight, to assess what we can safely remove, and see if we can make the code simpler overall
• Either you can identify a smaller scope where we can still see simplification opportunities (for eg, maybe classification is less complex?)
• Either we forget about that, and if you're still willing to contribute, we have other "Contributors welcome" issues that are (or should be 😀) well scoped and where your help would be very much appreciated!

Let me know!

cc @GBrelurut

[VishnoiAman777]

Hi @Valentin-Laurent,
Thank you for clarity on the scope. I’m still keen to tackle this issue in full scope. I’ll go into the codebase to map out where sample_weight is used, checked, or transformed, once done, let's discuss what can be safely removed, and how we can make code simpler.

[Valentin-Laurent]

Awesome @VishnoiAman777 , thank you!

[VishnoiAman777]

Hi @Valentin-Laurent,

I have been working on refactoring the codebase for _MapieRegressor, _MapieQuantileRegressor, and their usage in ConformalizedQuantileRegressor, CrossConformalRegressor, and JackknifeAfterBootstrapRegressor. I believe these methods can be simplified by making the following changes:

1. Replace _prepare_fit_params_and_sample_weight with _prepare_params:

   • The function _prepare_fit_params_and_sample_weight in the fit method of classes that inherit _MapieRegressor can be replaced by _prepare_params. We can just create a copy of fit_params and there is no need to separate out sample_weight from fit_params

2. Remove Explicit Passing of sample_weight:

   • We can remove the explicit passing of sample_weight from all functions in the _MapieRegressor class.

3. Remove Redundant Check in _check_fit_parameters:

   • In _check_fit_parameters, we can safely remove the line sample_weight, X, y = _check_null_weight(sample_weight, X, y) because the checks performed by _check_null_weight are inherently handled by sklearn.utils.validation._check_sample_weight during estimator.fit(X, y, **fit_params), and we don't explicitly need to select values curresponding to non-null sample_weights, since there loss function will go to 0. We can also safely remove any cast operation to Numpy array that's happening.

4. Rewrite _fit_oof_estimator:

   • The _fit_oof_estimator function needs to be slightly rewritten to handle sample weights more efficiently. We can't remove the usage of _fit_estimator because of the reasons that I have stated later on in the commend. But now sample_weight are used in this only one function and can be safely removed from everywhere else, this will make codebase cleaner and simpler.

     @staticmethod
     def _fit_oof_estimator(
         estimator: RegressorMixin,
         X: ArrayLike,
         y: ArrayLike,
         train_index: ArrayLike,
         **fit_params,
     ) -> RegressorMixin:
         X_train = _safe_indexing(X, train_index)
         y_train = _safe_indexing(y, train_index)
         if 'sample_weight' in fit_params:
             sample_weight = _check_sample_weight(fit_params.pop('sample_weight'), X)
         else:
             sample_weight = None
         if not (sample_weight is None):
             sample_weight = _safe_indexing(sample_weight, train_index)
         estimator = _fit_estimator(
             estimator,
             X_train,
             y_train,
             sample_weight=sample_weight,
             **fit_params
         )
         return estimator

However I have encountered one issue in the _fit_estimator function.

The _fit_estimator is not handling sample_weights effectively for sklearn.pipeline.Pipeline objects. Here's the implementation for _fit_estimator:

     def _fit_estimator(
         estimator: Union[RegressorMixin, ClassifierMixin],
         X: ArrayLike,
         y: ArrayLike,
         sample_weight: Optional[NDArray] = None,
         **fit_params,
     ) -> Union[RegressorMixin, ClassifierMixin]:
            fit_parameters = signature(estimator.fit).parameters
            supports_sw = "sample_weight" in fit_parameters
            if supports_sw and sample_weight is not None:
                estimator.fit(X, y, sample_weight=sample_weight, **fit_params)
            else:
                estimator.fit(X, y, **fit_params)
            return estimator

in case of Pipeline object sample_weight are never applied anywhere because sample_weight is not in parameters of Pipeline Object's fit method. We should apply sample_weights to each of the step in the Pipeline (it can be composed of single/multi estimators), that can accept it in case of Pipeline object. I have written some piece of code here to fix this:

if isinstance(estimator, Pipeline): 
         for _, step_estimator in estimator.steps:
                  fit_parameters = signature(step_estimator.fit).parameters 
                  supports_sw = "sample_weight" in fit_parameters 
                  if supports_sw and sample_weight is not None: 
                      step_estimator.fit(X, y, sample_weight=sample_weight, **fit_params) 
                 else:
                     step_estimator.fit(X, y, **fit_params) 
                    # Update X for the next step in the pipeline 
                 X = step_estimator.transform(X) 

And the same issue can happen with SplitConformalRegressor with Pipeline Object

• Currently, using SplitConformalRegressor with a Pipeline object and sample weights throws an error. Here's a sample code to reproduce the issue:

     X, y = make_regression(n_samples=100, n_features=5, random_state=42)
     y = y.reshape(-1, 1)
     mdl = Pipeline([
         ("poly", PolynomialFeatures(degree=2)),
         ("linear", LinearRegression())
     ])
     sample_weight = np.ones(X.shape[0])
     # This will throw an error because in the fit method it's directly doing estimator.fit(X, y, **fit_params) 
     mapie = SplitConformalRegressor(estimator=mdl, confidence_level=0.9, prefit=False)
     mapie.fit(X, y, {"sample_weight": sample_weight})

To address this, we should use _fit_estimator and make sure it works well with Pipeline objects too. We should also add a test case in case of SplitConformalRegressor for this(there is an existing test case called test_pipeline_compatibility for _MapeiRegressor so it get triggered for CrossConformalRegressor, but not for SplitConformalRegressor)

Please let me know if these changes make sense or if there are any errors in my hypothesis.
I am yet to start working on _MapieQuantileRegressor, and CrossConformalClassifier post this.