Repository: scikit-learn/scikit-learn
Pull request: BUG fix behaviour in confusion_matrix with with empty array-like as input
<!--
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#pull-request-checklist
-->

#### Reference Issues/PRs

Fixes #16432 

#### What does this implement/fix? Explain your changes.

Currently on master `confusion_matrix` raises a `ValueError` when `y_true` and `y_pred` are empty and `labels` is not `None`.  

```python
from sklearn.metrics import confusion_matrix

confusion_matrix([], [])
# returns a 0x0 numpy array of dtype int64

confusion_matrix([], [], labels=[True, False])
# raises ValueError
```
With the change in this PR, the second case will return an nxn numpy array of zeros.  This is correct with respect to the definition of confusion matrix given in the documentation.

#### Any other comments?

This change should be especially useful to users using `confusion_matrix` in automated reporting.  Automated reports will sometimes (correctly) be run on datasets of size 0.  In this case allowing `confusion_matrix` to correctly return an nxn matrix of zeros rather than raising will eliminate the need for the user to write a special case.



Problem statement:
confusion_matrix errors with empty y_true/y_pred
#### Describe the bug
When `confusion_matrix` is called with zero length `y_true` and `y_pred` and `labels` is not `None`.  A value error is raised `ValueError: At least one label specified must be in y_true`.

#### Steps/Code to Reproduce
call confusion matrix with zero length `y_true` and `y_pred` and labels specified.

```python
from sklearn.metrics import confusion_matrix

confusion_matrix([], [], labels=[True, False])
```
The error is raised even `labels` is specified but length zero and even if labels is numeric or if `sample_weight` is specified.

#### Expected Results
In the above case a 2x2 2d array of zeros.  In general if `n` labels are provided, an nxn d2 array of zeros.

#### Actual Results
A value error is raised.
```
ValueError: At least one label specified must be in y_true
```

#### Versions
Linux-5.3.0-28-generic-x86_64-with-debian-buster-sid
Python 3.7.3 (default, Aug 12 2019, 15:56:44) 
[GCC 7.4.0]
NumPy 1.16.0
SciPy 1.2.0
Scikit-Learn 0.19.2
Imbalanced-Learn "not installed"

Hints from issue discussion:
I think the explicit error is reasonable, as it likely suggest issues earlier in the data pipeline. 

What is the use case for returning a confusion matrix with zeros in this case?
> What is the use case for returning a confusion matrix with zeros in this case?

I think the main case is that a matrix of zeros _is_ the confusion matrix in this case.  `confusion_matrix([], [])` returns a 0x0 array rather than error which seems to be the correct behavior.

I'm a bit unclear of the condition being checked. https://github.com/scikit-learn/scikit-learn/blob/db85b1264c5b0d24a944a9312c4f5a0cb2b033c9/sklearn/metrics/_classification.py#L279
```
labels = np.asarray(labels)
if np.all([l not in y_true for l in labels]):
    raise ValueError("At least one label specified must be in y_true")
```
It seems the condition to check to raise a value error should be something like `any(y not in labels for y in y_true) or any(y not in labels for y in y_pred)`.  Currently if values appear in `y_true` or `y_pred` but not `labels` they are silently ignored which violates the expectation of `confusion_matrix` that `np.sum(confusion_matrix(y_true, y_pred)) == len(y_true)`
In fact `confusion_matrix` seems to have the behavior that I would expect (in regard to `y_true` and `y_pred` being empty) when lines 279 and 280 are removed.  Those lines also don't protect against (what is to me) unexpected behavior when values appear in `y_true` or `y_pred` that do no appear in `labels`.
Handling this case where labels is given seems reasonable by me.

I think that we have 2 points here:

> I think the main case is that a matrix of zeros is the confusion matrix in this case. confusion_matrix([], []) returns a 0x0 array rather than error which seems to be the correct behavior.

Managing this case is YAGNI. I would like to have a real use-case where this is actually an issue.

> Currently if values appear in y_true or y_pred but not labels they are silently ignored which violates the expectation of confusion_matrix that np.sum(confusion_matrix(y_true, y_pred)) == len(y_true)

For this case, I agree. Basically something like:

```python
from sklearn.metrics import confusion_matrix 
confusion_matrix([1, 1, 3, 2, 0], [1, 0, 0, 1, 2], labels=[1, 2, 3])         
array([[1, 0, 0],
       [1, 0, 0],
       [0, 0, 0]])
```

`0` is discarded which should not be the case I think. This could lead to wrong errors.
We should check that labels in `y_true` and `y_pred` should all be reported in `labels`.

> Managing this case is YAGNI. I would like to have a real use-case where this is actually an issue.

So the real use case is that I was writing tests for my code which reports multiple performance metrics including the confusion matrix.  One of the test cases was empty input as I wanted to make sure that the function would work correctly on the entire range of input (as much as it is possible to verify that).  That case failed due to the behavior of `confusion_matrix` leading me to write a special case into my code.  As for managing this case, the code actually works like expected (by me) if the check `if np.all([l not in y_true for l in labels]):` is simply removed.

Changing the behavior when some value in `y_true` or `y_pred` does not appear in labels, seems to be a somewhat larger behavior change.  In my mind raising an error in that case would be the more sound response, but I don't know exactly what the standards are for this project and gradual api evolution.

In either case all the behaviors being discussed seem wrapped up in a single test (that tests multiple things)
https://github.com/scikit-learn/scikit-learn/blob/fa9cf22b2792e63f9ff95cd417723b3359a2c689/sklearn/metrics/tests/test_classification.py#L890-L914

I've altered the code to remove the "if all label not in y_true" check and the only test that fails is the last assertion of the above test.  (Disclaimer: I did not run the full test suite, just `metrics/test/test_common.py` and `metrics/test/test_classification.py`).  Furthermore it seems that if we instead change the check in question to a "if any (y_true or y_pred) not in labels" check then again only the above test fails (though a different assertion).
> So the real use case is that I was writing tests for my code which reports multiple performance metrics including the confusion matrix. One of the test cases was empty input as I wanted to make sure that the function would work correctly on the entire range of input (as much as it is possible to verify that).

This is still not clear to me what the situation in which in practise, you get empty `y_pred`/`y_true`. Since you are writing tests, in which case are you expecting your user to call your function with empty `y_pred`/`y_true`?
If you have some groups where each group can have 0 or more instances that need a prediction.  The following is not my domain but the idea applies.  You could have online orders where you are trying to predict if jewelry purchases are a gift or not.  You want to be able to consider the confusion matrix at the level of an order.  An order may have 0 jewelry purchases.  Or perhaps you have an automatic daily report on travel insurance claims that you want to predict as legitimate or fraudulent.  On some days there may be simply no claims, but on days with severe weather there may be many.

In any case, making `confusion_matrix` be able to deal with zero length input when labels are specified will make it both consistent with itself (compared to when labels are not specified) and correct with respect to the documentation and the Wikipedia article linked in the documentation.
OK, thanks for the details. So this is more a convenience usage. I could imagine that it might be useful when making some kind of automated reporting (even if could actually avoid computing the confusion matrix).

I think a better way to solve the issue is to return as early as possible:

```python
if (not y_pred and not y_true) and labels:
    n_labels = len(labels)
    return np.zeros((n_labels, n_labels), dtype=np.int)
```

The second thing to consider is that `plot_confusion_matrix` should create a proper plot (especially in the normalize case since we will have division by zero).
Since normalization allows for row or column normalization, I think that's an issue that's already been considered.  It looks like right before returning there's a coercion of `nan` back to zero (if I'm understanding the `np.nan_to_num` function correctly).

https://github.com/scikit-learn/scikit-learn/blob/d591d8c467b23f81e7aa95e626b5b594ee674808/sklearn/metrics/_classification.py#L316-L323

So it sounds like there's some consensus on what the behavior should be when `y_true` and `y_pred` are empty and labels are specified.  It also sounds like there may be some agreement on changing the "if all label not in y_true then raise value error" to "if any (y_true or y_pred) not in labels then raise value error".  If those are both the case then I can put together a PR addressing those two changes.

Commit message: update behavior

Patch to review:
diff --git a/sklearn/metrics/_classification.py b/sklearn/metrics/_classification.py
index 1fa6086ff32a7..52df38518aaa9 100644
--- a/sklearn/metrics/_classification.py
+++ b/sklearn/metrics/_classification.py
@@ -274,6 +274,9 @@ def confusion_matrix(y_true, y_pred, labels=None, sample_weight=None,
 
     if labels is None:
         labels = unique_labels(y_true, y_pred)
+    elif len(y_true) == 0:
+        n_labels = len(labels)
+        return np.zeros((n_labels, n_labels), dtype=np.int)
     else:
         labels = np.asarray(labels)
         if np.all([l not in y_true for l in labels]):
diff --git a/sklearn/metrics/tests/test_classification.py b/sklearn/metrics/tests/test_classification.py
index 947ca047438d8..cc9aaaf34ba0b 100644
--- a/sklearn/metrics/tests/test_classification.py
+++ b/sklearn/metrics/tests/test_classification.py
@@ -914,6 +914,18 @@ def test_confusion_matrix_multiclass_subset_labels():
                          labels=[extra_label, extra_label + 1])
 
 
+@pytest.mark.parametrize(
+    'labels', (None, [0, 1], [0, 1, 2]),
+    ids=['labels None', 'binary classification', 'three way classification']
+)
+def test_confusion_matrix_on_zero_length_input(labels):
+    labels = [] if not labels else labels
+    n_classes = len(labels)
+    expected = np.zeros((n_classes, n_classes))
+    cm = confusion_matrix([], [], labels)
+    assert_array_equal(cm, expected)
+
+
 def test_confusion_matrix_dtype():
     y = [0, 1, 1]
     weight = np.ones(len(y))


Code review comments:
diff_hunk: @@ -914,6 +914,18 @@ def test_confusion_matrix_multiclass_subset_labels():
                          labels=[extra_label, extra_label + 1])
 
 
+@pytest.mark.parametrize(
+    'labels', (None, [0, 1], [0, 1, 2]),
+    ids=['labels None', 'binary classification', 'three way classification']
+)
+def test_confusion_matrix_on_zero_length_input(labels):
+    labels = [] if not labels else labels
+    n_classes = len(labels)
+    expected = np.zeros((n_classes, n_classes))
path: sklearn/metrics/tests/test_classification.py
text: @user1:
Just to be explicit inside the test.

```suggestion
    expected = np.zeros((n_classes, n_classes), dtype=np.int)
```

diff_hunk: @@ -274,6 +274,9 @@ def confusion_matrix(y_true, y_pred, labels=None, sample_weight=None,
 
     if labels is None:
         labels = unique_labels(y_true, y_pred)
+    elif len(y_true) == 0:
path: sklearn/metrics/_classification.py
text: @user1:
Since we are checking the array, I think this is more pythonic to do the following to check for emptyness.

```suggestion
    elif not y_true:
```

@author:
Ok, so it looks like after calling `_check_targets` and then immediately raising an error if `y_type` is not binary or multiclass we know that `y_true` and `y_pred` must be 1d numpy arrays.  I don't think that numpy arrays are truthy and falsey like lists are.
```python
import numpy as np

print(bool(np.array([])))
# prints false but raises a deprecation warning 
# suggesting `array.size > 0` instead
# in the future an error will be returned

print(bool(np.array([0])))
# prints False

print(bool(np.array([1])))
# prints True

print(bool(np.array([0, 0])))
# raises an error that the truth value of an array 
# with more than one element is ambiguous
```

So it sounds like the suggestion from numpy is to use `elif y_true.size == 0`.

@user1:
yep I thought that it was behaving as a list. so let's keep your code, this is the best solution I think

diff_hunk: @@ -914,6 +914,18 @@ def test_confusion_matrix_multiclass_subset_labels():
                          labels=[extra_label, extra_label + 1])
 
 
+@pytest.mark.parametrize(
+    'labels', (None, [0, 1], [0, 1, 2]),
+    ids=['labels None', 'binary classification', 'three way classification']
path: sklearn/metrics/tests/test_classification.py
text: @user1:
```suggestion
    ids=['None', 'binary', 'multiclass']
```