[MRG+1] label binarizer not used consistently in CalibratedClassifierCV by srivatsan-ramesh · Pull Request #7799 · scikit-learn/scikit-learn

srivatsan-ramesh · 2016-10-31T12:52:17Z

Reference Issue

What does this implement/fix? Explain your changes.

The LabelBinarizer() was not used consistently in CalibratedClassifierCV In the example provided in the issue #7796 , the when using LeaveOneOut the test set will have only one class, whereas the train set has two classes. Two different instances of the LabelBinarizer are used to encode the test and train set.
So, I have added a new argument 'classes' in the fit() function of _CalibratedClassifier class (which contains the unique classes of the test set), as I didn't know of any other method to solve this issue. If someone has a better solution I can change it.

jnothman

Please add a test

jnothman · 2016-10-31T13:39:35Z

sklearn/calibration.py

        return df, idx_pos_class

-    def fit(self, X, y, sample_weight=None):
+    def fit(self, X, y, sample_weight=None, classes=None):


Could we please move this parameter to the constructor?

srivatsan-ramesh · 2016-10-31T14:02:49Z

@jnothman Done!

jnothman · 2016-11-01T03:58:53Z

sklearn/tests/test_calibration.py

+def test_calibration_prob_sum():
+    """Test that sum of probabilities is 1"""
+    num_classes = 2
+    X, y = make_classification(n_samples=100, n_features=20,


Please make n_samples smaller to reduce test time.

jnothman · 2016-11-01T03:59:36Z

sklearn/tests/test_calibration.py

+
+
+def test_calibration_prob_sum():
+    """Test that sum of probabilities is 1"""


We don't use docstrings in tests because they make nose output harder to read. Make this a comment instead. Also comment that this is a non-regression test for issue #... as it's otherwise a bit obscure to use LOO.

jnothman · 2016-11-01T04:09:37Z

This otherwise looks good as a fix for the issue-as-presented. However, I think there are other oddities in the code here. LabelBinarizer is being used in CalibratedClassifierCV only to set classes_ and is then ignored. This assumes, therefore that the classes used by each base estimator will be the same. While that is true, I think, for all classifiers in scikit-learn, I'd feel more comfortable if CalibratedClassifierCV encoded all labels as integers using LabelEncoder, then used that encoded version thenceforth. It's unclear to me that _CalibratedClassifier, as a private class, should need to accept anything other than a contiguous sequence of 0-based integer labels for classes, and so should be able to take a required parameter n_classes and that be all. (This would also remove the somewhat expensive use of np.sum(y == class_) where bincount can instead be used with encoded y.) (retracted, see #7799 (comment))

It's also unclear to me that the current implementation correctly handles the case that classes are absent for some training sets.

jnothman · 2016-11-01T04:09:52Z

sklearn/calibration.py

                calibrated_classifier = _CalibratedClassifier(
-                    this_estimator, method=self.method)
+                    this_estimator, method=self.method,
+                    classes=np.unique(y[train]))


are you sure this should be y[train] and not y?

I m not sure. I thought that since the base_estimator is fitted with y[train], the base_estimator will assume that only np.unique(y[train]) classes exist, but still there will be a problem if y[test] has some extra classes. So, should I change it to y @jnothman ?

This is related to my concern that "It's also unclear to me that the current implementation correctly handles the case that classes are absent for some training sets."

Could you concoct a test case, with toy data and your own train-test splits, say, that checks this issue?

@jnothman added the test (and I hope this is how you expected it to be?)
I can make the changes which you have listed above to the class CalibratedClassifierCV. Do you want me to commit it in this PR or should I make another PR after this one is merged?

Let's see :)

jnothman · 2016-11-01T12:03:13Z

sklearn/tests/test_calibration.py

+    clf = LinearSVC(C=1.0)
+    clf_prob = CalibratedClassifierCV(clf, method="sigmoid", cv=LeaveOneOut())
+    clf_prob.fit(X, y)
+    probs = clf_prob.predict_proba(X)


I think this may be hiding the issue: is it an array of shape (10, 10) or of shape (10, 9)?

The shape of probs array is (10, 10), and BTW this worked only when I changed the classes parameter to self.classes_ instead of np.unique(y[train]).

are you able to drill down further and ensure that each constituent calibrated classifier is returning probability of 0 for a different class?

We may need to specially handle the case where a binarized label's data is all zeros.

Like you said, I also think that the current implementation does not handle the case where some classes are absent in training set.

Not necessarily. If the base estimator happens to have only been trained on a subset of the classes, the mapping between the columns of the base_estimator's decision function and those of that CalibratedClassifierCV should output is not so trivial .

@jnothman self.base_estimator.classes_ will be the class labels and we have to change that to integers with LabelEncoder.transform()(Already fitted with all the classes) and this should work?

Ridge isn't a classifier...

ignore that, you're not using ridge anymore :/

@jnothman Some test which was already present was using Ridge in CalibratedClassifierCV, that's why I asked you. Maybe I will change that test to some other classifier.

jnothman · 2016-11-01T13:14:00Z

And we can't assume always encoded labels in _CalibratedClassifier. To handle the 'prefit' case, we must allow for arbitrary classes_ in the base_estimator.

srivatsan-ramesh · 2016-11-01T14:34:42Z

@jnothman Can we use LabelEncoder instead of LabelBinarizer in CalibratedClassifierCV ? And replace np.any([np.sum(y == class_) < n_folds for class_ in self.classes_]) with np.any(np.bincount(lb.transform(y)) < n_folds) ?

amueller · 2016-11-01T19:12:15Z

yeah, I'm a bit confused why the LabelBinarizer is instantiated here in the first place... you can definitely replace it by LabelEncoder from what I see. (I haven't looked into your PR in detail though)

jnothman · 2016-11-01T22:51:35Z

LabelBinarizer is used because the calibration (though not the underlying estimator) is fit on a per-class basis.

jnothman · 2016-11-02T04:27:34Z

yes, I think that should work.

On 2 November 2016 at 14:25, Srivatsan notifications@github.com wrote:

@srivatsan-ramesh commented on this pull request.

In sklearn/tests/test_calibration.py
#7799:

clf_prob = CalibratedClassifierCV(clf, method="sigmoid", cv=LeaveOneOut())

clf_prob.fit(X, y)

probs = clf_prob.predict_proba(X)

assert_array_almost_equal(probs.sum(axis=1), np.ones(probs.shape[0]))

Test to check calibration works fine when train set in a test-train

split does not contain all classes

Since this test uses LOO, at each iteration train set will not contain a

class label

X = np.random.randn(10, 5)

y = np.arange(10)

clf = LinearSVC(C=1.0)

clf_prob = CalibratedClassifierCV(clf, method="sigmoid", cv=LeaveOneOut())

clf_prob.fit(X, y)

probs = clf_prob.predict_proba(X)

@jnothman https://github.com/jnothman self.base_estimator.classes_ will
be the class labels and we have to change that to integers with
LabelEncoder.transform()(Already fitted with all the classes) and this
should work?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7799, or mute the
thread
https://github.com/notifications/unsubscribe-auth/AAEz6xwZhG5CA5bnAv1BywKx1674AOldks5q6AKRgaJpZM4Kk-uD
.

srivatsan-ramesh · 2016-11-03T02:46:45Z

sklearn/tests/test_calibration.py

+    clf = LinearSVC(C=1.0)
+    clf_prob = CalibratedClassifierCV(clf, method="sigmoid", cv=LeaveOneOut())
+    clf_prob.fit(X, y)
+    probs = clf_prob.predict_proba(X)


@jnothman Can you check the tests? I couldn't think of any other way to test this.

I meant to actually go through each of calibrated_classifiers_ and run predict_proba to confirm that each leaves a different column with zero-probability. Or am I wrong to think that's correct behaviour?

Yes that can be checked

jnothman

Apparently my comment was left pending for a while

jnothman · 2016-11-02T13:46:52Z

sklearn/tests/test_calibration.py

+    clf = LinearSVC(C=1.0)
+    clf_prob = CalibratedClassifierCV(clf, method="sigmoid", cv=LeaveOneOut())
+    clf_prob.fit(X, y)
+    probs = clf_prob.predict_proba(X)


Ridge isn't a classifier...

srivatsan-ramesh · 2016-11-06T09:05:10Z

sklearn/calibration.py

+            idx_pos_class = self.label_encoder_.\
+                transform(self.base_estimator.classes_)
+        else:
+            idx_pos_class = np.arange(df.shape[1])


Since Ridge has a decision_function method can we not use it with CalibratedClassifierCV. I did this workaround for such cases. Is it fine or Ridge should not work with CalibratedClassifierCV. @jnothman

Regressors like Ridge should not work with CalibratedClassifierCV

jnothman

Otherwise LGTM!

jnothman · 2016-11-06T10:43:56Z

sklearn/tests/test_calibration.py

+
+    for i, calibrated_classifier in \
+            enumerate(cal_clf.calibrated_classifiers_):
+        assert_array_equal(calibrated_classifier.predict_proba(X)[:, i],


Let's make this slightly stronger by checking that this column and only this column is zero:

proba = calibrated_classifier.predict_proba(X) assert_array_equal(proba[:, i], np.zeros(len(y))) assert_equal(np.all(np.hstack([proba[:, :i], proba[:, i+1:]])), True)

jnothman · 2016-11-06T11:03:03Z

sklearn/calibration.py

        corresponds to Platt's method or 'isotonic' which is a
        non-parametric approach based on isotonic regression.

+    classes : array-like, shape (n_classes,)


Unimportant, given that this is a private class, but conventionally we'd add ", optional" to this line

jnothman · 2016-11-06T11:11:55Z

Please also add a what's new entry.

…ntain all labels

agramfort · 2016-11-08T06:09:22Z

thx @srivatsan-ramesh

jnothman · 2016-11-08T06:11:06Z

I think this can also be squeezed into 0.18.1, if the release manager agrees it's a worthwhile bug-fix (sorry for all the what's new hacking you need to do @amueller).

…CV (scikit-learn#7799) * label binarizer not used consistently in CalibratedClassifierCV * changed position of classes argument to make old tests run * moved parameter to constructor and added test * added test where train set doesnt have all classes * CalibratedClassifierCV can now handle cases where train set doesnt contain all labels * fixing flake error * fixing line lengths * removing np.full() * from __future__ import division for py2.7 * change is test file * added an extra test and removed a test with Ridge * stronger test * whats new entry

* tag '0.18.1': (144 commits) skip tree-test on 32bit do the warning test as we do it in other places. Replase assert_equal by assert_almost_equal in cosine test version bump 0.18.1 fix merge conflict mess in whatsnew add the python2.6 warning to 0.18.1 fix learning_curve test that I messed up in cherry-picking the "reentrant cv" PR. sync whatsnew with master [MRG] TST Ensure __dict__ is unmodified by predict, transform, etc (scikit-learn#7553) FIX scikit-learn#6420: Cloning decision tree estimators breaks criterion objects (scikit-learn#7680) Add whats new entry for scikit-learn#6282 (scikit-learn#7629) [MGR + 2] fix selectFdr bug (scikit-learn#7490) fixed whatsnew cherry-pick mess (somewhat) [MRG + 2] FIX LogisticRegressionCV to correctly handle string labels (scikit-learn#5874) [MRG + 2] Fixed parameter setting in SelectFromModel (scikit-learn#7764) [MRG+2] DOC adding separate `fit()` methods (and docstrings) for DecisionTreeClassifier and DecisionTreeRegressor (scikit-learn#7824) Fix docstring typo (scikit-learn#7844) n_features --> n_components [MRG + 1] DOC adding :user: role to whats_new (scikit-learn#7818) [MRG+1] label binarizer not used consistently in CalibratedClassifierCV (scikit-learn#7799) DOC : fix docstring of AIC/BIC in GMM ...

* releases: (144 commits) skip tree-test on 32bit do the warning test as we do it in other places. Replase assert_equal by assert_almost_equal in cosine test version bump 0.18.1 fix merge conflict mess in whatsnew add the python2.6 warning to 0.18.1 fix learning_curve test that I messed up in cherry-picking the "reentrant cv" PR. sync whatsnew with master [MRG] TST Ensure __dict__ is unmodified by predict, transform, etc (scikit-learn#7553) FIX scikit-learn#6420: Cloning decision tree estimators breaks criterion objects (scikit-learn#7680) Add whats new entry for scikit-learn#6282 (scikit-learn#7629) [MGR + 2] fix selectFdr bug (scikit-learn#7490) fixed whatsnew cherry-pick mess (somewhat) [MRG + 2] FIX LogisticRegressionCV to correctly handle string labels (scikit-learn#5874) [MRG + 2] Fixed parameter setting in SelectFromModel (scikit-learn#7764) [MRG+2] DOC adding separate `fit()` methods (and docstrings) for DecisionTreeClassifier and DecisionTreeRegressor (scikit-learn#7824) Fix docstring typo (scikit-learn#7844) n_features --> n_components [MRG + 1] DOC adding :user: role to whats_new (scikit-learn#7818) [MRG+1] label binarizer not used consistently in CalibratedClassifierCV (scikit-learn#7799) DOC : fix docstring of AIC/BIC in GMM ... Conflicts: removed sklearn/externals/joblib/__init__.py sklearn/externals/joblib/_parallel_backends.py sklearn/externals/joblib/testing.py

* dfsg: (144 commits) skip tree-test on 32bit do the warning test as we do it in other places. Replase assert_equal by assert_almost_equal in cosine test version bump 0.18.1 fix merge conflict mess in whatsnew add the python2.6 warning to 0.18.1 fix learning_curve test that I messed up in cherry-picking the "reentrant cv" PR. sync whatsnew with master [MRG] TST Ensure __dict__ is unmodified by predict, transform, etc (scikit-learn#7553) FIX scikit-learn#6420: Cloning decision tree estimators breaks criterion objects (scikit-learn#7680) Add whats new entry for scikit-learn#6282 (scikit-learn#7629) [MGR + 2] fix selectFdr bug (scikit-learn#7490) fixed whatsnew cherry-pick mess (somewhat) [MRG + 2] FIX LogisticRegressionCV to correctly handle string labels (scikit-learn#5874) [MRG + 2] Fixed parameter setting in SelectFromModel (scikit-learn#7764) [MRG+2] DOC adding separate `fit()` methods (and docstrings) for DecisionTreeClassifier and DecisionTreeRegressor (scikit-learn#7824) Fix docstring typo (scikit-learn#7844) n_features --> n_components [MRG + 1] DOC adding :user: role to whats_new (scikit-learn#7818) [MRG+1] label binarizer not used consistently in CalibratedClassifierCV (scikit-learn#7799) DOC : fix docstring of AIC/BIC in GMM ...

…CV (scikit-learn#7799) * label binarizer not used consistently in CalibratedClassifierCV * changed position of classes argument to make old tests run * moved parameter to constructor and added test * added test where train set doesnt have all classes * CalibratedClassifierCV can now handle cases where train set doesnt contain all labels * fixing flake error * fixing line lengths * removing np.full() * from __future__ import division for py2.7 * change is test file * added an extra test and removed a test with Ridge * stronger test * whats new entry

jnothman added the Bug label Oct 31, 2016

jnothman requested changes Oct 31, 2016

View reviewed changes

srivatsan-ramesh changed the title ~~label binarizer not used consistently in CalibratedClassifierCV~~ [MRG] label binarizer not used consistently in CalibratedClassifierCV Oct 31, 2016

raghavrv added the Waiting for Reviewer label Oct 31, 2016

jnothman requested changes Nov 1, 2016

View reviewed changes

jnothman reviewed Nov 1, 2016

View reviewed changes

srivatsan-ramesh commented Nov 3, 2016

View reviewed changes

jnothman reviewed Nov 5, 2016

View reviewed changes

srivatsan-ramesh commented Nov 6, 2016

View reviewed changes

jnothman requested changes Nov 6, 2016

View reviewed changes

jnothman changed the title ~~[MRG] label binarizer not used consistently in CalibratedClassifierCV~~ [MRG+1] label binarizer not used consistently in CalibratedClassifierCV Nov 6, 2016

srivatsan-ramesh added 10 commits November 6, 2016 23:30

label binarizer not used consistently in CalibratedClassifierCV

0832bda

changed position of classes argument to make old tests run

c57c4f1

moved parameter to constructor and added test

2b26c82

added test where train set doesnt have all classes

693f3a8

CalibratedClassifierCV can now handle cases where train set doesnt co…

b62cb4c

…ntain all labels

fixing flake error

4910004

fixing line lengths

ee98a8d

removing np.full()

95ba6ea

from __future__ import division for py2.7

5ae793c

change is test file

1e50a6c

srivatsan-ramesh added 3 commits November 6, 2016 23:30

added an extra test and removed a test with Ridge

466e6a0

stronger test

12be4ff

whats new entry

6d9b675

srivatsan-ramesh force-pushed the cdev2 branch from e3fba0d to 6d9b675 Compare November 6, 2016 18:02

jnothman approved these changes Nov 6, 2016

View reviewed changes

agramfort merged commit 41e1b8f into scikit-learn:master Nov 8, 2016

jnothman added this to the 0.18.1 milestone Nov 8, 2016

srivatsan-ramesh deleted the cdev2 branch November 8, 2016 16:46

jnothman mentioned this pull request Jun 13, 2017

Leave one out cross validation with CalibratedClassifierCV and LinearSVC #7796

Closed

lucyleeow mentioned this pull request Jul 1, 2020

Refactor CalibratedClassifierCV #17803

Closed

lucyleeow mentioned this pull request Jul 9, 2020

BUG CalibratedClassiferCV when train has less classes than test #17827

Closed

lucyleeow mentioned this pull request Jul 30, 2020

ENH Add libsvm-like calibration procedure to CalibratedClassifierCV #17856

Merged

lucyleeow mentioned this pull request Jul 25, 2024

BUG Problem when CalibratedClassifierCV train contains 2 classes but data contains more #29551

Open



		def test_calibration_prob_sum():
		"""Test that sum of probabilities is 1"""

Uh oh!

Conversation

srivatsan-ramesh commented Oct 31, 2016

Reference Issue

What does this implement/fix? Explain your changes.

Uh oh!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srivatsan-ramesh commented Oct 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnothman commented Nov 1, 2016 • 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!

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

jnothman commented Nov 1, 2016

Uh oh!

srivatsan-ramesh commented Nov 1, 2016

Uh oh!

amueller commented Nov 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnothman commented Nov 1, 2016

Uh oh!

jnothman commented Nov 2, 2016

@srivatsan-ramesh commented on this pull request.

Test to check calibration works fine when train set in a test-train

split does not contain all classes

Since this test uses LOO, at each iteration train set will not contain a

class label

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!

jnothman 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!

Choose a reason for hiding this comment

jnothman commented Nov 1, 2016 •

edited

Loading

amueller commented Nov 1, 2016 •

edited

Loading