Repository: scikit-learn/scikit-learn
Pull request: FIX renormalization of y_pred inside log_loss
<!--
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/main/CONTRIBUTING.md
-->

#### Reference Issues/PRs
<!--
Example: Fixes #1234. See also #3456.
Please use keywords (e.g., Fixes) to create link to the issues or pull requests
you resolved, so that they will automatically be closed when your pull request
is merged. See https://github.com/blog/1506-closing-issues-via-pull-requests
-->
Fixes #24515 

#### What does this implement/fix? Explain your changes.
- Raise a user warning if y_pred is not normalized inside log_loss
- Raise a deprecation warning for the eps parameter.

#### Any other comments?
- Tests to be added

<!--
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 log_loss renormalizes the predictions
### Describe the bug

`log_loss(y_true, y_pred)` renormalizes `y_pred` internally such that it sums to 1. This way, a really bad model, the predictions of which do not sum to 1, gets a better loss then it actually has.

### Steps/Code to Reproduce

```python
from scipy.special import xlogy
from sklearn.metrics import log_loss

y_true = [[0, 1]]
y_pred = [[0.2, 0.3]]

log_loss(y_true, y_pred)
```

### Expected Results

```python
-xlogy(y_true, y_pred).sum(axis=1)
```
Result: `1.2039728`

### Actual Results

Result: `0.5108256237659907`

### Versions

```shell
System:
    python: 3.9.14
   machine: macOS

Python dependencies:
      sklearn: 1.1.2
```

Hints from issue discussion:
can u share me the refernce of the code where the bug was there
@TomDLT I'd be interested in you opinion. We hit this in https://github.com/scikit-learn/scikit-learn/pull/24365#discussion_r976815764.
I feel like computing a log loss with probabilities not summing to one does not make sense, so I am ok with the renormalization.

> a really bad model, the predictions of which do not sum to 1

To me this is not a bad model (a model that has poor prediction accuracy), this is an incorrect model (a model that does not return results in the expected format), as would be a model that predicts negative probabilities. We could almost raise an error if the probabilities do not sum to one (or close), but I feel like it is simpler to just renormalize and compute the loss.
> I feel like computing a log loss with probabilities not summing to one does not make sense, so I am ok with the renormalization.

To be honest, I strongly think renormalizing in the metric is methodologically wrong because this way the metric is not measuring the model prediction anymore. The model should take care of it's own predictions (i.e. make them sum to 1), not the metric!

A metric is like a measuring device, say a scale (for measuring weight). We are measuring objects for a flight transport, so lighter is better. Renormalization is like measuring the weight of objects with their packaging removed. But the flight will have to carry the whole objects, with packaging included.
> We could almost raise an error if the probabilities do not sum to one (or close), but I feel like it is simpler to just renormalize and compute the loss.

I would be fine with raising a warning first and an error in the future.
I agree with @lorentzenchr that the current behavior is very surprising and therefore it's a bug to me.
What should be the behavior for larger `eps` values? For example, the following has `eps=0.1`:

```python
from sklearn.metrics import log_loss
import numpy as np

y_true = [0, 1, 2]
y_pred = [[0, 0, 1], [0, 1, 0], [0, 0, 1]]

eps = 0.1
log_loss(y_true, y_pred, eps=eps)
# 0.9330788879075577

# `log_loss` will use a clipped `y_pred` for computing the log loss:
np.clip(y_pred, eps, 1 - eps)
# array([[0.1, 0.1, 0.9],
#        [0.1, 0.9, 0.1],
#        [0.1, 0.1, 0.9]])
```

We could just validate the input, i.e. `np.isclose(y_pred.sum(axis=1), 1.0)`, but `log_loss` will use the clipped version for computation. For reference, here is the clipping:

https://github.com/scikit-learn/scikit-learn/blob/d52e946fa4fca4282b0065ddcb0dd5d268c956e7/sklearn/metrics/_classification.py#L2629-L2630

For me, I think that `eps` shouldn't be a parameter and should always be `np.finfo(y_pred.dtype).eps`. For reference, PyTorch's `binary_cross_entropy` [clips the log to -100 by default](https://github.com/pytorch/pytorch/blob/e47af44eb81b9cd0c3583de91b0a2d4f56a5cf8d/aten/src/ATen/native/Loss.cpp#L292-L297)
I agree with @thomasjpfan that `eps` should not be a parameter. In my opinion, the perfect default value is 0 (what is the use case of having it in the first place?). I'm also fine with `np.finfo(y_pred.dtype).eps`.
@lorentzenchr Would it be okay for me to work on this issue?
@OmarManzoor If the solution is clear to you, then yes.

Commit message: Remove renormalization of y_pred inside log_loss

Patch to review:
diff --git a/sklearn/metrics/_classification.py b/sklearn/metrics/_classification.py
index 9d1fd713f61c5..9dc828139d120 100644
--- a/sklearn/metrics/_classification.py
+++ b/sklearn/metrics/_classification.py
@@ -2617,7 +2617,16 @@ def log_loss(
     y_pred = check_array(
         y_pred, ensure_2d=False, dtype=[np.float64, np.float32, np.float16]
     )
-    eps = np.finfo(y_pred.dtype).eps if eps == "auto" else eps
+    if eps == "auto":
+        eps = np.finfo(y_pred.dtype).eps
+    else:
+        # TODO: Remove user defined eps in 1.4
+        warnings.warn(
+            "Setting the eps parameter is deprecated and will "
+            "be removed in 1.4. Instead eps will always have"
+            "a default value of `np.finfo(y_pred.dtype).eps`.",
+            FutureWarning,
+        )
 
     check_consistent_length(y_pred, y_true, sample_weight)
     lb = LabelBinarizer()
@@ -2680,7 +2689,14 @@ def log_loss(
 
     # Renormalize
     y_pred_sum = y_pred.sum(axis=1)
-    y_pred = y_pred / y_pred_sum[:, np.newaxis]
+    if (y_pred_sum != 1).any():
+        warnings.warn(
+            "The y_pred values are not normalized. Starting from 1.3 this"
+            "would result in an error.",
+            UserWarning,
+        )
+        y_pred = y_pred / y_pred_sum[:, np.newaxis]
+
     loss = -xlogy(transformed_labels, y_pred).sum(axis=1)
 
     return _weighted_sum(loss, sample_weight, normalize)


Code review comments:
diff_hunk: @@ -2680,7 +2689,14 @@ def log_loss(
 
     # Renormalize
     y_pred_sum = y_pred.sum(axis=1)
-    y_pred = y_pred / y_pred_sum[:, np.newaxis]
+    if (y_pred_sum != 1).any():
+        warnings.warn(
+            "The y_pred values are not normalized. Starting from 1.3 this"
+            "would result in an error.",
path: sklearn/metrics/_classification.py
text: @user1:
```suggestion
            "The y_pred values are do not sum to one. Starting from 1.3 this"
            "will result in an error.",
```

diff_hunk: @@ -2680,7 +2689,14 @@ def log_loss(
 
     # Renormalize
     y_pred_sum = y_pred.sum(axis=1)
-    y_pred = y_pred / y_pred_sum[:, np.newaxis]
+    if (y_pred_sum != 1).any():
path: sklearn/metrics/_classification.py
text: @user1:
This is too strict due to numerical precision. Something like
```python
if (!np.isclose(y_pred_sum, 1, rtol=1e-15, atol=5*eps).any():
```
might be enough.

@author:
According to this condition a value of 1.1 seems to be acceptable. Should we still normalize then for such a value, to preserve the tests?