(thanks for the review, you beat me to it while I was writing this long text)

@NicolasHug suggested the following refactoring (copied over):

I think we need to decouple 2 different things which are currently mixed in _CalibratedClassifier:
1. the chaining of the classifier and its corresponding calibrator, for prediction
2. the training of the calibrator

with suggested pseudocode: #16167 (comment)

Details of implementation in this PR:

• _get_predictions performs the same as the old _preproc. It returns predictions for X and index of classes present in X
• _fit_calibrator performs the same as the old _CalibratedClassifier.fit(). It fit calibrator(s) and returns a _CalibratedClassiferPipeline instance.
• New class _CalibratedClassiferPipeline. Instance consists of a fitted classifier and a list of fitted calibrators for every cv (or single calibrator when ensemble option implemented). _CalibratedClassiferPipeline.predict_proba() performs the same as old _CalibratedClassifier.predict_proba(). It returns the calibrated probas (for one cv fold), shape (n_samples, n_classes)
• As before CalibratedClassifierCV.predict_proba averages the probas from each cv fold.

Classes
I changed one thing not related to the refactoring: how to we keep track of classes. We need to keep track of classes in the case where y[test] contains less classes than y[train] (original PR). Changes:

• First, I think we can replace LabelBinarizer with LabelEncoder here:
  

  scikit-learn/sklearn/calibration.py

  Line 130 in fd23727

  le = LabelBinarizer().fit(y)

This is the only place that le is used and AFAICT LabelBinarizer.fit(y).classes_ would be the same as LabelEncoder.fit(y).classes_.

• The PR scale_params for linear models #779 added classes to _CalibratedClassifier and instantiates a new LabelEncoder at every step. I'm not sure if this is the best solution. I think it would be best to pass the fitted LabelEncoder instance instead of passing classes and fitting a new LabelEncoder instance every time.

(Edited for succinctness)

Do you think it would make sense to open another PR that branches off this one and that implements the new calibration procedure? It might make it easier to judge whether the refactoring that I proposed is appropriate

Happy to do this but could you describe the steps? Locally, I know how to make branch (e.g., 'add_ensemble') that branches off this branch (e.g., 'refactor'). Would I then just push 'add_ensemble' to make a new PR? Is the purpose such that you can look at the diff between 'add_ensemble' and 'refactor' ?

Would I then just push 'add_ensemble' to make a new PR?

yes, the new branch would be just like any other branch. It would not branch from master, unlike most branches we use, but other than that it's really just a branch

Is the purpose such that you can look at the diff between 'add_ensemble' and 'refactor' ?

Honestly I'm not sure my suggestion of having 2 PRs makes sense. The thing is, right now it's hard to review the refactoring because we cannot really tell whether it will help implementing the ensemble option. So I suggested having 2 PRs so that we can take a look at the "final" implementation as well, but maybe we should just go for that right away? It's really up to you what your preferred workflow would be

No problem @NicolasHug, happy to make a branch from this branch. I can practice my git!

The thing is, right now it's hard to review the refactoring because we cannot really tell whether it will help implementing the ensemble option.

I think you were right here. The refactoring needs to be re-jigged for it to work/help the implementation (so it doesn't use 'predictions_in_X' - from #16167). However, even with re-jigging, I can't think of a good way. This is what I got but I think it uses more memory (by saving the outputs form each cv fold) the current implementation and I don't think it's very elegant:

def _get_predictions(base_estimator, X, cv, ensemble,
                     base_estimator_supports_sw, label_encoder_):
    # returns a list of prediction tuples of form (`df`, `pos_class_indices`, `test_indicies`)
    # for `ensemble=True` list would contain 1 tuple, for `ensemble=False` the
    # list wold contain a tuple for each cv
    # Note cv loop done here
    # The `test_indicies` are required, since we need `sample_weight[test]` to fit the calibration
    # and the cv loop is inside here.


class CalibratedClassifierCV(BaseEstimator, ClassifierMixin,
                             MetaEstimatorMixin):
    def fit(self, X, y, sample_weight=None):
        # Checking
        # `_get_predictions`. Loop through list of prediction tuples and perform `_fit_calibrator`
        # return list of `_CalibratedClassiferPipeline` instances.

Any suggestions?

Sorry I don't understand why _get_predictions needs cv or base_estimator_supports_sw: these are fitting-related parameters, and my understanding is that _get_predictions only predicts?

Yes I guess they don't. I just I wanted to avoid repetition but it is not necessary. We can do, as with the original PR:

class CalibratedClassifierCV():
    ...
    if ensemble:
        # do cv for loop as before
    else:
        # use `cross_val_predict` and fit classifier (some repetition here)

I guess my question is did you just want an implementation without the use of predictions_in_X? I think we can do this by changing how _fit_calibrator() works - though this would get messy.

We can do, as with the original PR:...

I think this is what I had in mind yes. From what I can tell, what we need to change is mostly _CalibratedClassifier. Not so much CalibratedClassifierCV. But it's easy to miss things when you're not the one implementing, so you'll be the judge

I guess my question is did you just want an implementation without the use of predictions_in_X?

yes

I think we can do this by changing how _fit_calibrator() works - though this would get messy.

My suggestion (pasted below) doesn't work? My "vision" is that when ensemble is True you pass X and you call _fit_calibrator inside the manual CV loop. And when ensemble is False, _fit_calibrator is called only once, and you pass df directly (df would basically be the output of cross_val_predict). X and df are mutually exclusive.

def _fit_calibrator(fitted_classifier, method, y, X=None, df=None):
    # Take a fitted classifier as input, fit a calibrator, and return the
    # corresponding pipeline.
    # df is mutually exclusive with X
    # y should always be passed
    # This should handle the multiclass case too

    if df is None:
        df = fitted_classifier.decision_function(X)  # or predict_proba
    
    calibrator = _get_regressor(method)
    calibrator.fit(df, y)

Thanks, I was making it more difficult than it needs to be. First attempt here: #17856

I will amend documentation and add tests once the implementation is okay.