ENH Add Multiclass Brier Score Loss#22046
Conversation
Co-authored-by: Olivier Grisel <[email protected]>
Co-authored-by: Olivier Grisel <[email protected]>
There was a problem hiding this comment.
+1 for merge once https://github.com/scikit-learn/scikit-learn/pull/22046/files#r1943364780 is addressed (and conflicts are resolved).
@lorentzenchr do you agree with the way this PR evolved, in particular the points I raised in #22046 (comment)?
ogrisel
left a comment
ogrisel
left a comment
There was a problem hiding this comment.
Still +1 for merge (I cannot approve the PR in github because I am the creator of the PR).
doc/whats_new/upcoming_changes/sklearn.metrics/22046.feature.rst
Outdated
Show resolved
Hide resolved
lorentzenchr
left a comment
lorentzenchr
left a comment
There was a problem hiding this comment.
I would have preferred to have all the non-related input validation and test thing changes in a separate PR.
| if name in METRICS_REQUIRE_POSITIVE_Y: | ||
| y_true, y_pred = _require_positive_targets(y_true, y_pred) | ||
| always_symmetric = True | ||
| for _ in range(5): |
There was a problem hiding this comment.
A comment would help: why the loop? (make lucky test passes very unlikely)
sklearn/metrics/tests/test_common.py
Outdated
| if always_symmetric: # pragma: no cover | ||
| raise ValueError(f"{name} seems to be symmetric") |
There was a problem hiding this comment.
| if always_symmetric: # pragma: no cover | |
| raise ValueError(f"{name} seems to be symmetric") | |
| if not always_symmetric: | |
| raise ValueError(f"{name} seems to be asymmetric") |
There should be a test for this, e.g. applying test_not_symmetric_metric to log_loss and test for a fail.
There was a problem hiding this comment.
The meta test test_symmetry_tests in 6de5e13 checks test_symmetric_metric and test_not_symmetric_metric.
sklearn/metrics/_classification.py
Outdated
| For :math:`N` observations labeled from :math:`C` possible classes, the Brier | ||
| score is defined as: | ||
|
|
||
| .. math:: | ||
| \\frac{1}{N}\\sum_{i=1}^{N}\\sum_{c=1}^{C}(y_{ic} - \\hat{p}_{ic})^{2} |
There was a problem hiding this comment.
If I remember correctly, we try to avoid LaTeX in docstrings and just link to the user guide. If LaTeX then only in the the Notes section (this may be a numpy thing) @glemaitre my know better.
There was a problem hiding this comment.
The math are moved to the Notes section in 58e5f18. If you feel that's too redundant with the User Guide I can remove the Notes section.
Co-authored-by: Christian Lorentzen <[email protected]> Co-authored-by: Olivier Grisel <[email protected]>
|
Thanks for the reviews @thomasjpfan and @lorentzenchr. I think the PR is ready for a final round of reviews. |
Yes, sorry that we mix refactoring the |
Co-authored-by: Christian Lorentzen <[email protected]>
If you intent to do that, I would very much like to have the classification metrics better structured, i.e. putting log loss and brier score at the top where they belong. This might also help with not needing to define things twice. |
Co-authored-by: Varun Aggarwal <[email protected]> Co-authored-by: Antoine Baker <[email protected]>
Co-authored-by: Varun Aggarwal <[email protected]> Co-authored-by: Antoine Baker <[email protected]>
6 participants
Resolves #16055.
This PR updates #18699 by @aggvarun01 after a merge with
mainand resolves merge conflicts. I do not have the permissions to push directly in the original branch and opening a sub-PR pointing to #18699 would lead to an unreadable diff because of the one-year merge sync.I also added a changelog entry and demonstrate the new function in the multiclass calibration example.
@aggvarun01 if you want feel free to pull the last commit from this commit from this branch to your branch. Alternatively we can finalize the review here.