Repository: scikit-learn/scikit-learn
Pull request: [MRG+1] fixed OOB_Score bug for bagging classifiers.
Fixes #8933

<!--
Thanks for contributing a pull request! Please ensure you have taken a look at
the contribution guidelines: https://github.com/scikit-learn/scikit-learn/blob/master/CONTRIBUTING.md#Contributing-Pull-Requests
-->
#### Reference Issue
<!-- Example: Fixes #1234 -->


#### What does this implement/fix? Explain your changes.


#### Any other comments?


<!--
Please be aware that we are a loose team of volunteers so patience is
necessary; assistance handling other issues is very welcome. We value
all user contributions, no matter how minor they are. If we are slow to
review, either the pull request needs some benchmarking, tinkering,
convincing, etc. or more likely the reviewers are simply busy. In either
case, we ask for your understanding during the review process.
For more information, see our FAQ on this topic:
http://scikit-learn.org/dev/faq.html#why-is-my-pull-request-not-getting-any-attention.

Thanks for contributing!
-->


Problem statement:
BUG: BaggingClassifier.oob_score_ should not change with class label
Let us compute the oob score of a bagged classifier.

```python
import numpy as np
import pandas as pd
from sklearn.ensemble import BaggingClassifier
from sklearn.neighbors import KNeighborsClassifier

N = 50
randState = 5
label = 'Label'
features = ['A','B','C']

labels = np.random.randint(3, size = N) - 1
df = pd.DataFrame( labels , index=range(N), columns=[label] )
for col in features:
    df[col] = df[label] + 0.01 * np.random.rand( N )

clf = BaggingClassifier(base_estimator = KNeighborsClassifier(), n_estimators = 10, oob_score = True, random_state = randState )
clf.fit(df[features], df[label])
print clf.oob_score_
```

Here, clf.oob_score_=0.0.

Now, you would not expect that the OOB accuracy is a function of the class labels...

```python
df.loc[ df[label] == -1 , label ] = 2
clf = BaggingClassifier(base_estimator = KNeighborsClassifier(), n_estimators = 10, oob_score = True, random_state = randState )
clf.fit(df[features], df[label])
print clf.oob_score_
```

Now, clf.oob_score_=1.0.

Clearly, OOB score should not be a function of the labels arbitrarily chosen for the classes.

sklearn.__version__: '0.18.1'
numpy.__version__: '1.11.3'

Hints from issue discussion:
I don't think BaggingClassifier uses bincount over the labels. I've not yet
grokked your issue here entirely, though

On 25 May 2017 at 01:03, JohnNapier <notifications@github.com> wrote:

> Let us compute the oob score of a bagged classifier.
>
> import numpy as npimport pandas as pdfrom sklearn.ensemble import BaggingClassifierfrom sklearn.neighbors import KNeighborsClassifier
>
> N = 50
> randState = 5
> label = 'Label'
> features = ['A','B','C']
>
> labels = np.random.randint(3, size = N) - 1
> df = pd.DataFrame( labels , index=range(N), columns=[label] )for col in features:
>     df[col] = df[label] + 0.01 * np.random.rand( N )
>
> clf = BaggingClassifier(base_estimator = KNeighborsClassifier(), n_estimators = 10, oob_score = True, random_state = randState )
> clf.fit(df[features], df[label])print clf.oob_score_
>
> Surprisingly, clf.oob_score_=0.0.
>
> Now, you would not expect that the OOB accuracy is a function of the class
> labels...
>
> df.loc[ df[label] == -1 , label ] = 2
> clf = BaggingClassifier(base_estimator = KNeighborsClassifier(), n_estimators = 10, oob_score = True, random_state = randState )
> clf.fit(df[features], df[label])print clf.oob_score_
>
> Now clf.oob_score_=1.0.
>
> My guess is, this bug occurs because BaggingClassifier relies on
> numpy.bincount, which expects all labels to be positive integers.
>
> sklearn.*version*: '0.18.1'
> numpy.*version*: '1.11.3'
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/scikit-learn/scikit-learn/issues/8933>, or mute the
> thread
> <https://github.com/notifications/unsubscribe-auth/AAEz65isxCbbsh3bTIz2wcKd0BsfKjcYks5r9Ea-gaJpZM4NlPjo>
> .
>

Thanks @jnothman. The numpy.bincount comment is just a suspicion. 

In any case, do you agree that clf.oob_score_ should not be impacted by the chosen class labels? 
It appears the bug is here, in class BaggingClassifier(BaseBagging, ClassifierMixin):

```python
def _set_oob_score(self, X, y):
        oob_score = accuracy_score(y, classes_.take(np.argmax(predictions,axis=1)))
```

argmax is not giving the labels but rather the positions, which doesn’t map back to the original labels.
In fact, if the labels are [0,1,27] , you’ll get a third value for oob, which is also wrong.

Suggestion: BaggingClassifier should compute oob_score_ like RandomForestClassifier, which works fine.
thanks, a test and a patch is welcome

On 25 May 2017 1:38 am, "JohnNapier" <notifications@github.com> wrote:

> It appears the bug is here, in class BaggingClassifier(BaseBagging,
> ClassifierMixin):
>
> def _set_oob_score(self, X, y):
>         oob_score = accuracy_score(y, classes_.take(np.argmax(predictions,axis=1)))
>
> argmax is not giving the labels but rather the positions, which doesn’t
> map back to the original labels.
> In fact, if the labels are [0,1,27] , you’ll get a third value for oob,
> which is also wrong.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/scikit-learn/scikit-learn/issues/8933#issuecomment-303763147>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAEz60lGoa_lB8Y0MmC5oZDyh7HKqsHWks5r9E7cgaJpZM4NlPjo>
> .
>

Commit message: white space

Patch to review:
diff --git a/sklearn/ensemble/bagging.py b/sklearn/ensemble/bagging.py
index 711d089d057d9..be55ec03206c9 100644
--- a/sklearn/ensemble/bagging.py
+++ b/sklearn/ensemble/bagging.py
@@ -609,8 +609,7 @@ def _set_oob_score(self, X, y):
 
         oob_decision_function = (predictions /
                                  predictions.sum(axis=1)[:, np.newaxis])
-        oob_score = accuracy_score(y, classes_.take(np.argmax(predictions,
-                                                              axis=1)))
+        oob_score = accuracy_score(y, np.argmax(predictions, axis=1))
 
         self.oob_decision_function_ = oob_decision_function
         self.oob_score_ = oob_score
diff --git a/sklearn/ensemble/tests/test_bagging.py b/sklearn/ensemble/tests/test_bagging.py
index 0170a3fa2262f..ad5963e096a7f 100644
--- a/sklearn/ensemble/tests/test_bagging.py
+++ b/sklearn/ensemble/tests/test_bagging.py
@@ -723,3 +723,37 @@ def test_max_samples_consistency():
                                 max_features=0.5, random_state=1)
     bagging.fit(X, y)
     assert_equal(bagging._max_samples, max_samples)
+
+
+def test_set_oob_score():
+    # Make sure the oob_score doesn't change when the labels change
+    # See: https://github.com/scikit-learn/scikit-learn/issues/8933
+    N = 50
+    randState = 5
+    x = []
+    y = []
+    # Needs to maintain order of labels in case there are
+    # cases of equal predictions when running argmax
+    dict1 = {-1: 0, 0: 1, 1: 2}
+    dict2 = {-1: 'a', 0: 'b', 1: 'c'}
+
+    label1 = np.random.randint(3, size=N) - 1
+    label2 = [dict1[l] for l in label1]
+    label3 = [dict2[l] for l in label1]
+
+    df = np.tile(label1, (3, 1)).T
+    for col in range(3):
+        df[:, col] = df[:, col] + 2 * np.random.rand(N)
+
+    # Try 3 possibilities
+    for label in [label1, label2, label3]:
+        clf = BaggingClassifier(base_estimator=KNeighborsClassifier(),
+                                n_estimators=10, oob_score=True,
+                                random_state=randState)
+        clf.fit(df, label)
+        x.append(clf.oob_score_)
+
+    for i in range(len(x)):
+        y.append(x[0])
+
+    assert_equal(x, y)


Code review comments:
diff_hunk: @@ -723,3 +723,37 @@ def test_max_samples_consistency():
                                 max_features=0.5, random_state=1)
     bagging.fit(X, y)
     assert_equal(bagging._max_samples, max_samples)
+
+
+def test_set_oob_score():
+    # Make sure the oob_score doesn't change when the labels change
+    # See: https://github.com/scikit-learn/scikit-learn/issues/8933
+    N = 50
path: sklearn/ensemble/tests/test_bagging.py
text: @user1:
As a non-regression test, I don't see why this is any better than asserting that

```python
BaggingClassifier(oob_score=True).fit([[0], [1]] * 5, ['A', 'B'] * 5).oob_score_
```

is approximately 1... or, at a minimum, not 0, which the current bug gives.

@author:
I figured testing a "failing" label integer set ([-1,0,1]), a "standard" label set ([0,1,2]), and an alphanumeric label set (['a','b,'c']) would sufficiently exhaustive. Whatever your preference.

diff_hunk: @@ -723,3 +723,37 @@ def test_max_samples_consistency():
                                 max_features=0.5, random_state=1)
     bagging.fit(X, y)
     assert_equal(bagging._max_samples, max_samples)
+
+
+def test_set_oob_score():
path: sklearn/ensemble/tests/test_bagging.py
text: @user1:
perhaps `test_set_oob_score_label_encoding`

@author:
I can change the test name to that if it's preferred