Skip to content

FEA add binary_classification_curve #30134

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

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

SuccessMoses
Copy link
Contributor

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Fixes #16470

Any other comments?

  • In sklearn/metrics/_ranking.py, changed the name of the function _binary_clf_curve to binary_classifcation_curve without changing the body. I also changed test functions like test_binary_clf_curve_multiclass_error without changing the body
  • det_curve, roc_curve and precision_recall_curve call this function, so I updated the name of the function in the body
  • I added examples in the docstring of the function

Copy link

github-actions bot commented Oct 22, 2024

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 3094eca. Link to the linter CI: here

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'd also need to add this in api_reference.py under the right section to have it rendered in the docs properly.

@glemaitre you happy with the name?

@glemaitre glemaitre changed the title Added binary_classification_curve from _binary_clf_curve FEA Added binary_classification_curve from _binary_clf_curve Nov 5, 2024
@glemaitre glemaitre changed the title FEA Added binary_classification_curve from _binary_clf_curve FEA add binary_classification_curve Nov 5, 2024
@glemaitre
Copy link
Member

I think I'm fine with the name. I was looking if can have the word counts in the name of the function but it starts to be really long. So I would be OK with the proposed name.

SuccessMoses and others added 8 commits November 6, 2024 09:45
Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>
Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>
Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>
Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM.

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
@adrinjalali adrinjalali requested a review from glemaitre November 7, 2024 09:06
@SuccessMoses
Copy link
Contributor Author

@adrinjalali There is an issue with numpydoc validation in the binary_classification_curve function, the error is RT03: Return value has no description. Do you know why this is? I already documented the return value of the function.

@adrinjalali
Copy link
Member

When you look at the CI log, this is the error, not the return:

[gw0] linux -- Python 3.12.7 /usr/share/miniconda/envs/testvenv/bin/python
810         Decreasing score values.
811 
812     Examples
813     -------
814     >>> import numpy as np
815     >>> from sklearn.metrics import binary_classification_curve
816     >>> y_true = np.array([0, 0, 1, 1])
817     >>> y_scores = np.array([0.1, 0.4, 0.35, 0.8])
818     >>> fps, tps, thresholds = binary_classification_curve(y_true, y_scores)
819     >>> fps
Expected:
    array([0, 1, 1, 2])
Got:
    array([0., 1., 1., 2.])

You just need to fix the output to floats

@SuccessMoses
Copy link
Contributor Author

You just need to fix the output to floats

I fixed this one. But this is the log I was talking about


E           array([0.8, 0.4, 0.35, 0.1])
E           
E           # Errors
E           
E            - RT03: Return value has no description
E            - RT03: Return value has no description
E            - RT03: Return value has no description
E            - RT03: Return value has no description
E            - RT03: Return value has no description
E            - RT03: Return value has no description
E            - RT03: Return value has no description
E            - RT03: Return value has no description
E            - RT03: Return value has no description
E            - RT03: Return value has no description
E            - RT03: Return value has no description
E            - RT03: Return value has no description
E            - RT03: Return value has no description

function_name = 'sklearn.metrics._ranking.binary_classification_curve'
msg        = '\n\n/home/vsts/work/1/s/sklearn/metrics/_ranking.py\n\nTested function: sklearn.metrics._ranking.binary_classificatio...3: Return value has no description\n - RT03: Return value has no description\n - RT03: Return value has no description'
request    = <FixtureRequest for <Function test_function_docstring[sklearn.metrics._ranking.binary_classification_curve]>>
res        = {'deprecated': False, 'docstring': 'Calculate true and false positives per binary classification threshold.\n\nParamet...n'), ('RT03', 'Return value has no description'), ...], 'file': '/home/vsts/work/1/s/sklearn/metrics/_ranking.py', ...}

This test tests.test_docstrings.py::test_function_docstring[sklearn.metrics._ranking.binary_classification_curve] is failing.

@adrinjalali
Copy link
Member

That's indeed very odd. @StefanieSenger @lucyleeow would you have an idea here?

Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a new functionality, we also need to:

  • amend an example where it makes sense to use this feature. These examples are part of the gallery.
  • add the reference into the "See Also" sections of other metrics such as the other curves function and the confusion matrix function at least.
  • the function should also be discussed the user guide documentation in the metric section.

Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>
@glemaitre glemaitre added this to the 1.7 milestone Nov 18, 2024
@glemaitre
Copy link
Member

glemaitre commented Nov 25, 2024

By looking at the example change, I went back to the issue to understand the real use case here. So it seems that users are interested to get the entry of the confusion matrix to be able to compute different metrics of interest.

I'm wondering that what we have here is not enough since we only return the TP and FP and we don't provide any information about the negatives. Eg. computing the two samples Kolmogorov-Smirnov test (https://en.wikipedia.org/wiki/Kolmogorov%E2%80%93Smirnov_test) could require these data.

@adrinjalali would you think that if we make this function public, we should also return the TN and FN for completness?

@glemaitre glemaitre requested review from adrinjalali and removed request for glemaitre November 25, 2024 17:26
@adrinjalali
Copy link
Member

Yeah I think that makes sense @glemaitre .

Also related, we probably want to get #15522 merged

@SuccessMoses
Copy link
Contributor Author

@glemaitre it is possible to compute the negatives from the positives. Since the tps array is increasing, false negatives are given by tps[-1] - tps. Similarly, true negatives are given by fps[-1] - fps. _binary_clf_curve does not return negatives because they are easy to calculate.

@glemaitre
Copy link
Member

@SuccessMoses You will need an additional statistic most probably to infer the four statistics. However, on a user perspective, it is really annoying to have to carry on some code to compute the negative part. I would like just to call the function, get my counts and then deal with them. It is a nicer user experience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Make _binary_clf_curve a "public" method?
4 participants