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

fix(areaScores): consistency b/w aupr auroc #26176

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

Conversation

a-parida12
Copy link

Reference Issues/PRs

Fixes #24381

What does this implement/fix? Explain your changes.

  • replace ValueError with Userwarning
  • update -0.0 with 0.0

@glevv
Copy link
Contributor

glevv commented Jul 28, 2023

Hi! You need to update tests so that it will expect warning and not value error

@lucyleeow
Copy link
Member

@a-parida12 would you still like to work on this PR?

@a-parida12
Copy link
Author

@lucyleeow could you please help me out, by pointing to me what needs to be done to be able to merge it? I updated the changes from the main to the current branch.

@github-actions
Copy link

github-actions bot commented Oct 25, 2023

❌ Linting issues

This PR is introducing linting issues. Here's a summary of the issues. Note that you can avoid having linting issues by enabling pre-commit hooks. Instructions to enable them can be found here.

You can see the details of the linting issues under the lint job here


black

black detected issues. Please run black . locally and push the changes. Here you can see the detected issues. Note that running black might also fix some of the issues which might be detected by ruff. Note that the installed black version is black=23.3.0.


error: cannot format /home/runner/work/scikit-learn/scikit-learn/sklearn/metrics/_ranking.py: Cannot parse: 387:8:         warnings.warn(

Oh no! 💥 💔 💥
906 files would be left unchanged, 1 file would fail to reformat.

ruff

ruff detected issues. Please run ruff --fix --show-source . locally, fix the remaining issues, and push the changes. Here you can see the detected issues. Note that the installed ruff version is ruff=0.1.11.


error: Failed to parse sklearn/metrics/_ranking.py:387:9: Unexpected token 'warnings'
sklearn/metrics/_ranking.py:387:9: E999 SyntaxError: Unexpected token 'warnings'
    |
385 |             "Only one class present in y_true. ROC AUC score "
386 |             "is not defined in that case."
387 |         warnings.warn(
    |         ^ E999
388 |             "Only one class present in y_true. ROC AUC score is not defined in that "
389 |             "case. The value 0.0 is returned.",
    |

Found 1 error.

mypy

mypy detected issues. Please fix them locally and push the changes. Here you can see the detected issues. Note that the installed mypy version is mypy=1.3.0.


sklearn/metrics/_ranking.py:384: error: '(' was never closed  [syntax]
Found 1 error in 1 file (errors prevented further checking)

Generated for commit: 5757d7e. Link to the linter CI: here

@lucyleeow
Copy link
Member

@a-parida12
Copy link
Author

@lucyleeow thanks for pointing it out to me... I will get working on it.

@a-parida12
Copy link
Author

@lucyleeow @glevv this pull request is ready!

Copy link
Contributor

@glevv glevv left a comment

Choose a reason for hiding this comment

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

I think you should add a UndefinedMetricWarning type here instead of UserWarning

@a-parida12
Copy link
Author

@glevv cool I will update the UserWarning to the UndefinedMetricWarning

@glevv
Copy link
Contributor

glevv commented Nov 15, 2023

Will need a reviewer on this one
ping @Micky774 @adrinjalali @ogrisel @glemaitre

with pytest.raises(ValueError):
metric(y1_row, y2_row)
if "roc_auc" not in name:
with pytest.raises(ValueError):
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, which metrics currently hit this branch?

Copy link
Member

Choose a reason for hiding this comment

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

The following:

  • adjusted_balanced_accuracy_score
  • balanced_accuracy_score
  • cohen_kappa_score
  • d2_tweedie_score
  • hinge_loss
  • matthews_corrcoef_score
  • max_error
  • mean_compound_poisson_deviance
  • mean_gamma_deviance
  • mean_normal_deviance
  • mean_poisson_deviance
  • normalized_confusion_matrix
  • ovo_roc_auc
  • ovr_roc_auc
  • top_k_accuracy_score
  • unnormalized_confusion_matrix
  • weighted_ovo_roc_auc
  • weighted_ovr_roc_auc

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to sanity check then, do we want to propogate the preference for a warning over an error to any of these as well?

@glemaitre glemaitre self-requested a review November 15, 2023 17:19
@glemaitre
Copy link
Member

I would prefer to have to separate pull-request:

  • one with the issue related to the negative zero
  • one related to the warning

Indeed, we will need two different entries in the changelog

sklearn/metrics/_ranking.py Outdated Show resolved Hide resolved
sklearn/metrics/_ranking.py Show resolved Hide resolved
@@ -363,7 +363,10 @@ def test_roc_curve_toydata():
with pytest.warns(UndefinedMetricWarning, match=expected_message):
tpr, fpr, _ = roc_curve(y_true, y_score)

with pytest.raises(ValueError):
expected_message = (
Copy link
Member

Choose a reason for hiding this comment

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

You will need to update the message and type of warning.

with pytest.raises(ValueError):
metric(y1_row, y2_row)
if "roc_auc" not in name:
with pytest.raises(ValueError):
Copy link
Member

Choose a reason for hiding this comment

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

The following:

  • adjusted_balanced_accuracy_score
  • balanced_accuracy_score
  • cohen_kappa_score
  • d2_tweedie_score
  • hinge_loss
  • matthews_corrcoef_score
  • max_error
  • mean_compound_poisson_deviance
  • mean_gamma_deviance
  • mean_normal_deviance
  • mean_poisson_deviance
  • normalized_confusion_matrix
  • ovo_roc_auc
  • ovr_roc_auc
  • top_k_accuracy_score
  • unnormalized_confusion_matrix
  • weighted_ovo_roc_auc
  • weighted_ovr_roc_auc

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@glemaitre
Copy link
Member

The CIs are failing.

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

Successfully merging this pull request may close these issues.

Inconsistency in AUC ROC and AUPR API
5 participants