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

[MRG+1] label binarizer not used consistently in CalibratedClassifierCV #7799

Merged
merged 13 commits into from Nov 8, 2016

Conversation

Projects
None yet
5 participants
@srivatsan-ramesh
Contributor

srivatsan-ramesh commented Oct 31, 2016

Reference Issue

#7796

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 jnothman added the Bug label Oct 31, 2016

@jnothman

Please add a test

@@ -289,7 +291,7 @@ def _preproc(self, X):
return df, idx_pos_class
def fit(self, X, y, sample_weight=None):
def fit(self, X, y, sample_weight=None, classes=None):

This comment has been minimized.

@jnothman

jnothman Oct 31, 2016

Member

Could we please move this parameter to the constructor?

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

@srivatsan-ramesh

This comment has been minimized.

Contributor

srivatsan-ramesh commented Oct 31, 2016

@jnothman Done!

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,

This comment has been minimized.

@jnothman

jnothman Nov 1, 2016

Member

Please make n_samples smaller to reduce test time.

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

This comment has been minimized.

@jnothman

jnothman Nov 1, 2016

Member

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

This comment has been minimized.

Member

jnothman commented Nov 1, 2016

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.

@@ -175,7 +175,8 @@ def fit(self, X, y, sample_weight=None):
this_estimator.fit(X[train], y[train])
calibrated_classifier = _CalibratedClassifier(
this_estimator, method=self.method)
this_estimator, method=self.method,
classes=np.unique(y[train]))

This comment has been minimized.

@jnothman

jnothman Nov 1, 2016

Member

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

This comment has been minimized.

@srivatsan-ramesh

srivatsan-ramesh Nov 1, 2016

Contributor

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 comment has been minimized.

@jnothman

jnothman Nov 1, 2016

Member

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?

This comment has been minimized.

@srivatsan-ramesh

srivatsan-ramesh Nov 1, 2016

Contributor

@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?

This comment has been minimized.

@jnothman

jnothman Nov 1, 2016

Member

Let's see :)

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

This comment has been minimized.

@jnothman

jnothman Nov 1, 2016

Member

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

This comment has been minimized.

@srivatsan-ramesh

srivatsan-ramesh Nov 1, 2016

Contributor

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]).

This comment has been minimized.

@jnothman

jnothman Nov 1, 2016

Member

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

This comment has been minimized.

@jnothman

jnothman Nov 1, 2016

Member

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

This comment has been minimized.

@srivatsan-ramesh

srivatsan-ramesh Nov 1, 2016

Contributor

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

This comment has been minimized.

@jnothman

jnothman Nov 2, 2016

Member

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 .

This comment has been minimized.

@srivatsan-ramesh

srivatsan-ramesh Nov 2, 2016

Contributor

@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?

This comment has been minimized.

@jnothman

jnothman Nov 5, 2016

Member

Ridge isn't a classifier...

This comment has been minimized.

@jnothman

jnothman Nov 5, 2016

Member

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

This comment has been minimized.

@srivatsan-ramesh

srivatsan-ramesh Nov 6, 2016

Contributor

@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

This comment has been minimized.

Member

jnothman commented Nov 1, 2016

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

This comment has been minimized.

Contributor

srivatsan-ramesh commented Nov 1, 2016

@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

This comment has been minimized.

Member

amueller commented Nov 1, 2016

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

This comment has been minimized.

Member

jnothman commented Nov 1, 2016

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

@jnothman

This comment has been minimized.

Member

jnothman commented Nov 2, 2016

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
.

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

This comment has been minimized.

@srivatsan-ramesh

srivatsan-ramesh Nov 3, 2016

Contributor

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

This comment has been minimized.

@jnothman

jnothman Nov 5, 2016

Member

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?

This comment has been minimized.

@srivatsan-ramesh

srivatsan-ramesh Nov 6, 2016

Contributor

Yes that can be checked

@jnothman

Apparently my comment was left pending for a while

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

This comment has been minimized.

@jnothman

jnothman Nov 5, 2016

Member

Ridge isn't a classifier...

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

This comment has been minimized.

@srivatsan-ramesh

srivatsan-ramesh Nov 6, 2016

Contributor

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

This comment has been minimized.

@jnothman

jnothman Nov 6, 2016

Member

Regressors like Ridge should not work with CalibratedClassifierCV

@jnothman

Otherwise LGTM!

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

This comment has been minimized.

@jnothman

jnothman Nov 6, 2016

Member

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)
@@ -253,6 +257,11 @@ class _CalibratedClassifier(object):
corresponds to Platt's method or 'isotonic' which is a
non-parametric approach based on isotonic regression.
classes : array-like, shape (n_classes,)

This comment has been minimized.

@jnothman

jnothman Nov 6, 2016

Member

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

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

@jnothman

This comment has been minimized.

Member

jnothman commented Nov 6, 2016

Please also add a what's new entry.

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

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@agramfort

This comment has been minimized.

Member

agramfort commented Nov 8, 2016

thx @srivatsan-ramesh

@jnothman jnothman added this to the 0.18.1 milestone Nov 8, 2016

@jnothman

This comment has been minimized.

Member

jnothman commented Nov 8, 2016

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).

@srivatsan-ramesh srivatsan-ramesh deleted the srivatsan-ramesh:cdev2 branch Nov 8, 2016

amueller added a commit to amueller/scikit-learn that referenced this pull request Nov 9, 2016

[MRG+1] label binarizer not used consistently in CalibratedClassifier…
…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

sergeyf added a commit to sergeyf/scikit-learn that referenced this pull request Feb 28, 2017

[MRG+1] label binarizer not used consistently in CalibratedClassifier…
…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

Sundrique added a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017

[MRG+1] label binarizer not used consistently in CalibratedClassifier…
…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

yarikoptic added a commit to yarikoptic/scikit-learn that referenced this pull request Jul 27, 2017

Merge tag '0.18.1' into releases
* 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
  ...

yarikoptic added a commit to yarikoptic/scikit-learn that referenced this pull request Jul 27, 2017

Merge branch 'releases' into dfsg
* 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

yarikoptic added a commit to yarikoptic/scikit-learn that referenced this pull request Jul 27, 2017

Merge branch 'dfsg' into debian
* 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
  ...

NelleV added a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017

[MRG+1] label binarizer not used consistently in CalibratedClassifier…
…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

paulha added a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017

[MRG+1] label binarizer not used consistently in CalibratedClassifier…
…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

maskani-moh added a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017

[MRG+1] label binarizer not used consistently in CalibratedClassifier…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment