Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Make it possible to pass an arbitrary classifier as method for CalibratedClassifierCV #22010

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aperezlebel
Copy link
Contributor

Reference Issues/PRs

Fixes #21280

What does this implement/fix? Explain your changes.

  • Implement a third calibration method using a custom classifier in CalibratedClassifierCV.
  • Update existing tests with this new method.
  • Create new tests specific to this method.
  • Extend at least one calibration example to demonstrate this new capability.
  • Create a new example highlighting the advantages of this method compared to sigmoid and isotonic.
  • Update the Probability calibration page in the user guide with a paragraph on this method.

Any other comments?

This PR replaces #21992 as issue #21280 was updated.

@aperezlebel
Copy link
Contributor Author

Note that I had to update the settings of the existing tests listed below to make them pass with this new method.

  • tests/test_calibration.py::test_calibration: I increased the number of total samples from 200 to 250, and the number of training samples from 100 to 200. Otherwise, the brier score was not decreasing compared to the uncalibrated one as required by this test.
  • tests/test_calibration.py::test_calibration_multiclass: I increased the number of total samples from 500 to 750. Otherwise the brier score was not in the 10% range of the uncalibrated one as required by this test.

Other tests pass with this new method without changes.

I don't think this is a problem specific to this new method as these tests were quite arbitrary in the first place both in the choice of the 10% margin and in the requirement that the brier score should decrease. For example, the existing test test_calibration_multiclass fails on the isotonic and sigmoid methods when increasing the number of samples from 500 to 1000. Similarly, test_calibration fails on isotonic and sigmoid when decreasing the number of training samples from 100 to 50 and the total number of samples from 200 to 75.

@ogrisel
Copy link
Member

ogrisel commented Jan 5, 2022

To avoid having to increase the dataset sizes in the test, maybe you can try with a more regularized model:

gbdt_calibrator = HistGradientBoostingClassifier(monotonic_cst=[1], max_leaf_nodes=5, max_iter=10)

Another advantage would be to reduce the test duration.

if that does not work, then it's fine to increase the test data size as you did.

EDIT: alternatively you can use LogisticRegression(C=1e12) instead of HistGradientBoostingClassifier(monotonic_cst=[1]) everywhere in the tests. Since this should be equivalent to sigmoid calibration, the existing test should all pass unchanged.

Then HistGradientBoostingClassifier(monotonic_cst=[1], max_leaf_nodes=5, max_iter=10) would only be used in the new example.

# If binary classification, only return proba of the positive class
if probas.shape[1] == 2:
return probas[:, 1]
return probas
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a new test to cover for the multiclass case, for instance using LogisticRegression(C=1e12).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would require unplugging the One-vs-Rest reduction logic typically used for sigmoid and isotonic calibration.


# Discard samples with null weights
mask = sample_weight > 0
X, y, sample_weight = X[mask], y[mask], sample_weight[mask]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this? I would just pass the weights unchanged to the underlying estimator.

"""

def __init__(self, method):
self.method = clone(method)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather move the call to clone to the fit method to be consistent with other meta-estimators in scikit-learn (even though this one is not meant to be directly used by scikit-learn users).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rename method to estimator and then in fit do:

    self.estimator_ = clone(self.estimator)

and then fit that instead.

@ogrisel
Copy link
Member

ogrisel commented Jan 5, 2022

Could you please add a new test that checks the approximate equivalence of fitting with method="sigmoid" and method=LogisticRegression(C=1e12) by inspecting that the a_ and b_ attributes of the_SigmoidCalibration calibrator match the coef_ and intercept_ attributes of the estimator_ attribute of the fitted _CustomCalibration instance.

@ogrisel
Copy link
Member

ogrisel commented Jan 13, 2022

Another PR to keep in mind: #17541. If #17541 gets merged before #22010, then it will be nice to adapt #22010 accordingly.

@glemaitre glemaitre self-requested a review November 8, 2022 14:46
@glemaitre glemaitre removed their request for review January 11, 2023 12:57
@glemaitre
Copy link
Member

@aperezlebel Once you addressed @ogrisel comments, you can ping me for a review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make it possible to pass an arbitrary probablistic classifier as method for CalibratedClassifierCV
3 participants