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

Jump to bottom

[MRG] Add min_tpr parameter to roc_auc_score, allow both min_tpr and max_fpr #19751

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

[MRG] Add min_tpr parameter to roc_auc_score, allow both min_tpr and max_fpr #19751

wants to merge 20 commits into from

Conversation

Fengjun-Wang
Copy link

@Fengjun-Wang Fengjun-Wang commented Mar 22, 2021 •
edited

Reference Issues/PRs

Fixes #11668
Fixes #11725
See also #3273
See also #3840

What does this implement/fix? Explain your changes.

  • Add min_tpr option for sklearn.metrics.roc_auc_score. Make the function allowing both min_tpr and max_fpr limitations at the same time.
  • Add unit test and common tests for the new argument
  • Add an example documentation to explain the standardized partial AUC calculation

@Fengjun-Wang Fengjun-Wang changed the title [WIP] Add min_tpr parameter to roc_auc_score, allow both min_tpr and max_fpr #11725 [WIP] Add min_tpr parameter to roc_auc_score, allow both min_tpr and max_fpr Mar 22, 2021
@Fengjun-Wang
Copy link
Author

Fengjun-Wang commented Mar 24, 2021 •
edited

Could anyone help me with the sklearn/metrics/test_common.py? I don't understand the errors.

  • For example, one error is
    test_multiclass_sample_weight_invariance[partial_roc_auc_min_tpr] ValueError: Partial AUC computation not available in multiclass setting, 'min_tpr' must be set to `None`, received `min_tpr=1` instead
    I already defined the partial_roc_auc_min_tpr doesn't support multiclass (in the METRIC_UNDEFINED_MULTICLASS dict).
    And the ValueError is expected when user try to apply partial AUC for multiclass classification results.

  • Another error example is
    test_binary_sample_weight_invariance[partial_roc_auc_min_tpr] ValueError: Expected min_tpr in range [0, 1), got: 1,
    In the function of partial AUC, we set range [0, 1) for min_tpr, and throw out error for out of range inputs. I don't understand why it is an error...

Do I need to change any core function of test_common?

jnothman
jnothman reviewed Mar 25, 2021
View changes
Copy link
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

Hth

sklearn/metrics/_ranking.py Outdated Show resolved Hide resolved
sklearn/metrics/tests/test_common.py Outdated Show resolved Hide resolved
@Fengjun-Wang Fengjun-Wang changed the title [WIP] Add min_tpr parameter to roc_auc_score, allow both min_tpr and max_fpr [MRG] Add min_tpr parameter to roc_auc_score, allow both min_tpr and max_fpr Mar 25, 2021
@Fengjun-Wang
Copy link
Author

Fengjun-Wang commented Jul 6, 2021 •
edited

Is anyone reviewing the MRG? I'd appreciate your comments and make improvements if needed.
@xrobin you have comments on issue 3273, where you mentioned the importance of adding both FPR and TPR constraints.
@albertcthomas I noticed you are active in MRG 3840, and gave lots of code reviews.
Maybe you are also interested in this MRG?

@cmarmo
Copy link
Member

cmarmo commented Jan 17, 2022 •
edited

Hi @Fengjun-Wang I'm sorry your PR got lost at some point. If you are still interested in it, do you mind synchronizing with upstream? If not, this is totally understandable. I will put the PR in the "Help Wanted" pool. Thanks for your patience and your work so far.

@Fengjun-Wang
Copy link
Author

Fengjun-Wang commented Jan 28, 2022 •
edited

Hi @Fengjun-Wang I'm sorry your PR got lost at some point. If you are still interested in it, do you mind synchronizing with upstream? If not, this is totally understandable. I will put the PR in the "Help Wanted" pool. Thanks for your patience and your work so far.

@cmarmo Thanks for your attention. I will do it in Feb.

@cmarmo
Copy link
Member

cmarmo commented Feb 19, 2022

Thanks @Fengjun-Wang for following-up. The lint fail is black related. You can solve it following the instructions here.

@Fengjun-Wang
Copy link
Author

Thanks @Fengjun-Wang for following-up. The lint fail is black related. You can solve it following the instructions here.

Thank you! I fixed the formatting and Test coverage issues. Now it passed all the checks.
Hope it will be reviewed soon :)

@cmarmo cmarmo mentioned this pull request May 6, 2022
Closed
@cmarmo cmarmo added this to the 1.2 milestone May 9, 2022
@cmarmo cmarmo removed the Enhancement label May 9, 2022
@cmarmo cmarmo modified the milestone: 1.2 May 9, 2022
@ogrisel ogrisel self-requested a review May 30, 2022 12:54
@ogrisel
Copy link
Member

ogrisel commented May 30, 2022

Hi @Fengjun-Wang, sorry for the wait. I pushed a cosmetic change to the example to trigger the CI again to check the HTML rendering of the example and to ease the review.

@ogrisel
Copy link
Member

ogrisel commented May 30, 2022

I merged the main branch to this PR to see if it fixes the Circle CI build.

ogrisel
ogrisel reviewed May 30, 2022
View changes
doc/whats_new/v0.24.rst Outdated
- |Enhancement| Add `min_tpr` parameter in :func:`metrics.roc_auc_score`,
allowing both `max_fpr` and `min_tpr` restrictions on partial AUC calculation.
Copy link
Member

Choose a reason for hiding this comment

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

This changelog entry needs to be moved to v1.2.rst and may I suggest the following rephrasing to make the change more explicit:

Suggested change
- |Enhancement| Add `min_tpr` parameter in :func:`metrics.roc_auc_score`,
allowing both `max_fpr` and `min_tpr` restrictions on partial AUC calculation.
- |Enhancement| Add `min_tpr` parameters to :func:`metrics.roc_auc_score`
allowing to set restrictions on the True Positive Rates to compute partial
ROC AUC when used in conjunction with the existing `max_fpr` parameter.

Copy link
Author

Choose a reason for hiding this comment

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

thx. I am changing it and going to push

examples/model_selection/partial_auc.py Outdated
interpolate=True,
color="green",
)
plt.show()
Copy link
Member

Choose a reason for hiding this comment

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

This does not render in sphinx-gallery. I believe that the filename of the example needs to be renamed to examples/model_selection/plot_partial_auc.py for this to work.

Copy link
Member

Choose a reason for hiding this comment

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

Actually we should probably merge this into the existing plot_roc.py as additional sections to avoid proliferation of example in the gallery which makes it hard to navigate.

Copy link
Author

Choose a reason for hiding this comment

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

I renamed it. Can you review, and let me know do we want to merge it in plot_roc? plot_roc is a quite long section, maybe we just need an additional link to tell the user we have partial auc? @ogrisel

Copy link
Author

Choose a reason for hiding this comment

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

Black hook reformat my file, and that reformat seems cause the file violate PEP8's max line length. Like stated here"psf/black#1178

Copy link
Author

Choose a reason for hiding this comment

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

I allowed black changes, since otherwise the Linting has errors

@jeremiedbb
Copy link
Member

We won't have time to finish the review on this one before the 1.2 release. Moving it to 1.3

@jeremiedbb jeremiedbb modified the milestones: 1.2, 1.3 Nov 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnothman jnothman jnothman left review comments

@ogrisel ogrisel ogrisel left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Enhancement module:metrics Waiting for Reviewer
Projects
None yet
Milestone
1.3
Development

Successfully merging this pull request may close these issues.

roc_auc_score min_tpr parameter?
5 participants
@Fengjun-Wang @cmarmo @ogrisel @jeremiedbb @jnothman