-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
FIX compute precision-recall at 100% recall #23214
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
FIX compute precision-recall at 100% recall #23214
Conversation
You will need to fix failing tests. Basically, it should be docstring test in most cases. |
@glemaitre I added 3 commits:
|
Please add an entry to the change log at |
@@ -855,11 +855,11 @@ def precision_recall_curve(y_true, probas_pred, *, pos_label=None, sample_weight | |||
>>> precision, recall, thresholds = precision_recall_curve( | |||
... y_true, y_scores) | |||
>>> precision | |||
array([0.66666667, 0.5 , 1. , 1. ]) | |||
array([0.5 , 0.66666667, 0.5 , 1. , 1. ]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should update the docstring of thresholds
to define properly n_thresholds
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from these 2 changes, LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes. LGTM
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
It seems the doc is failing, but I cannot understand why:
nothing wrong at line 105. |
For sure this is not linked with your change. We can ignore it. |
I merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @stephanecollot
It went very smoothly, I'm happy I did this first contribution, thank you @glemaitre. |
@stephanecollot I started a POC on the subject: #21211 The idea is to use cross-validation to get uncertainty bounds. I will probably find time to carry on some work in the next release. However, we will need to breakdown the PR into smaller PR to facilitate the review process:
|
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> Co-authored-by: jeremiedbb <jeremiedbb@yahoo.fr>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> Co-authored-by: jeremiedbb <jeremiedbb@yahoo.fr>
Reference Issues/PRs
Fixes #23213
What does this implement/fix? Explain your changes.
Remove the unnecessary dropping.
Any other comments?
Full disclosure, this PR modifies
precision_recall_curve()
that is only used by_binary_uninterpolated_average_precision()
that is only used byaverage_precision_score()
scikit-learn/sklearn/metrics/_ranking.py
Line 205 in 24106c2
I think average_precision_score() should not be impacted by this change, and its is tested 54 times in unit tests