Skip to content
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

Make zero_division parameter consistent in the different metric #29048

Open
5 tasks
glemaitre opened this issue May 19, 2024 · 10 comments
Open
5 tasks

Make zero_division parameter consistent in the different metric #29048

glemaitre opened this issue May 19, 2024 · 10 comments
Milestone

Comments

@glemaitre
Copy link
Member

glemaitre commented May 19, 2024

This is an issue to report the step to actually take over the work of @marctorsoc in #23183 and split the PR into smaller one to facilitate the review process.

The intend is to make the zero_division parameter consistent across different metrics in scikit-learn. In this regards, we have the following TODO list:

All those items have been addressed in #23183 and can be extracted in individual PRs. The changelog presenting the changes should acknowledge @marctorsoc.

In addition, we should investigate #27047 and check if we should add the zero_division parameter to the precision_recall_curve and roc_curve as well. This might add two additional items to the list above.

@glemaitre glemaitre added this to the 1.6 milestone May 19, 2024
@github-actions github-actions bot added the Needs Triage Issue requires triage label May 19, 2024
@glemaitre glemaitre added Enhancement and removed Needs Triage Issue requires triage labels May 19, 2024
@glemaitre
Copy link
Member Author

@StefanieSenger you might be interested in looking at some of the item.

@erikhuck
Copy link

+1 for this!

@Jaimin020
Copy link

I am working on task-1 (Introduce the zero_division parameter to the accuracy_score function when y_true and y_pred are empty.).

@erikhuck
Copy link

erikhuck commented Jun 5, 2024

Thank you @Jaimin020 ! I needed that recently and had to write a wrapper of the accuracy function.

@StefanieSenger
Copy link
Contributor

The intend of this issue is not quite clear to me. @glemaitre, could you clarify:

#23183 is about to be merged for the last 1,5 years, but is difficult to review. This issue is supposed to facilitate this process by splitting it into different pull-requests. However, it only lists tasks that would enhance the original PR (or add to it) and depend on it to be merged (except matthew_corr_coeff that had already been added to the old PR by your request).

  1. How would adding those new PRs help maintainers to decide about the original PR?
  2. Would those new PRs also be mergable if the original PR was in the end not adopted?

I can also see that the communication on in the original PR has been very frustrating for its author, because it was unclear, what he needs to deliver in order to push the reviewing process forwards. We should at least define this very clearly right now. This will help everybody involved (and everybody who wants to get involved) know what to expect and be efficient.

@StefanieSenger
Copy link
Contributor

I realised that the solution lies in first splitting up the original PR as proposed by the author.

After that, the other tasks from the task list above can be tackled.

I will open PRs that split up the original PR during the next days.

@glemaitre
Copy link
Member Author

I realised that the solution lies in first #23183 (comment) as proposed by the author.

Exactly.

I will open PRs that split up the original PR during the next days.

Do not hesitate to tackle a single PR first such that we make sure to get reviews.

@StefanieSenger
Copy link
Contributor

StefanieSenger commented Jun 6, 2024

Do not hesitate to tackle a single PR first such that we make sure to get reviews.

Good point. So I will start with this: "PR to add zero_division=np.nan to precision, recall, f1, fbeta_score, precision_recall_fscore_support and classification_report" (Edit: This was already done.)

I had missed the fact that the original PR had already mostly been adopted by #25531 and the remaining tasks are actually only those listed in the issue above. The original PR had now been closed by the author to avoid confusion (mine 🤣 ).

I will take care of the inividual PR for the cohen_kappa_score function.

@StefanieSenger
Copy link
Contributor

I am also working on adding the zero_division param to class_likelihood_ratios().

@Jaimin020
Copy link

I have created a pull request for task-1 (#29213), but the 'codecov/patch' check is failing. Could someone please review it and advise me on how to pass this check?

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

No branches or pull requests

4 participants