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

[MRG+1] Partial AUC #3840

Merged
merged 3 commits into from Mar 1, 2018

Conversation

Projects
None yet
5 participants
@Alexander-N
Contributor

Alexander-N commented Nov 7, 2014

This needs better tests, any ideas how to make them?

Issue #3273
partial AUC: integrate the ROC curve until chosen maximal FPR and standardize it to lie between 0.5 and 1.

@jnothman jnothman changed the title from [WIP] adress Issue #3273 to [WIP] Partial AUC Nov 8, 2014

@raghavrv raghavrv referenced this pull request Feb 3, 2015

Closed

Implement Average SGD #543

@lesshaste

This comment has been minimized.

Show comment
Hide comment
@lesshaste

lesshaste Sep 1, 2017

This seems like a useful addition. What happened to it?

lesshaste commented Sep 1, 2017

This seems like a useful addition. What happened to it?

@albertcthomas

This comment has been minimized.

Show comment
Hide comment
@albertcthomas

albertcthomas Sep 21, 2017

Contributor

This could be useful for anomaly/outlier/novelty detection as well as we are especially interested in the performance of the scoring function (e.g. decision_function) in the regions corresponding to small FPR values.

Contributor

albertcthomas commented Sep 21, 2017

This could be useful for anomaly/outlier/novelty detection as well as we are especially interested in the performance of the scoring function (e.g. decision_function) in the regions corresponding to small FPR values.

@albertcthomas albertcthomas referenced this pull request Sep 21, 2017

Closed

partial AUC #3273

@Alexander-N

This comment has been minimized.

Show comment
Hide comment
@Alexander-N

Alexander-N Sep 21, 2017

Contributor

I stopped working on this since I wasn't sure how to test this properly. If anybody could give me some hints I would pick it up again.

Contributor

Alexander-N commented Sep 21, 2017

I stopped working on this since I wasn't sure how to test this properly. If anybody could give me some hints I would pick it up again.

Show outdated Hide outdated sklearn/metrics/tests/test_ranking.py Outdated
Show outdated Hide outdated sklearn/metrics/tests/test_ranking.py Outdated
@serv-inc

This comment has been minimized.

Show comment
Hide comment
@serv-inc

serv-inc Jan 31, 2018

Works fine with 0.18.2. File is ranking.py.txt

How can this be brought to a finish?

@Alexander-N: would you prefer if you edited some testing code into this, or are you no longer interested? @jnothman probably meant that you should add a new function to test_ranking.py with a few different test cases.

  • result for max_fpr=1 is the same as not setting the parameter
  • results for max_fpr=0.9 as compared to max_fpr=1, etc

serv-inc commented Jan 31, 2018

Works fine with 0.18.2. File is ranking.py.txt

How can this be brought to a finish?

@Alexander-N: would you prefer if you edited some testing code into this, or are you no longer interested? @jnothman probably meant that you should add a new function to test_ranking.py with a few different test cases.

  • result for max_fpr=1 is the same as not setting the parameter
  • results for max_fpr=0.9 as compared to max_fpr=1, etc
@Alexander-N

This comment has been minimized.

Show comment
Hide comment
@Alexander-N

Alexander-N Jan 31, 2018

Contributor

Sorry for being unresponsive. I will look into it this weekend.

Contributor

Alexander-N commented Jan 31, 2018

Sorry for being unresponsive. I will look into it this weekend.

@Alexander-N Alexander-N changed the title from [WIP] Partial AUC to [MRG] Partial AUC Feb 4, 2018

@Alexander-N

This comment has been minimized.

Show comment
Hide comment
@Alexander-N

Alexander-N Feb 22, 2018

Contributor

I addressed the comments, this is ready for further review. Just wanted to make that clear to attract reviewers :-).

Contributor

Alexander-N commented Feb 22, 2018

I addressed the comments, this is ready for further review. Just wanted to make that clear to attract reviewers :-).

@jnothman

Can you add this as a metric for common testing in metrics/tests/test_common.py? This way you can check its basic properties and invariances.

Show outdated Hide outdated sklearn/metrics/ranking.py Outdated
Show outdated Hide outdated sklearn/metrics/ranking.py Outdated
Show outdated Hide outdated sklearn/metrics/ranking.py Outdated
@albertcthomas

A partial review. For the tests, I was told very recently by @jnothman that we should use assert instead of assert_equal to check equality.

Show outdated Hide outdated sklearn/metrics/ranking.py Outdated
Show outdated Hide outdated sklearn/metrics/ranking.py Outdated
Show outdated Hide outdated sklearn/metrics/tests/test_ranking.py Outdated
Show outdated Hide outdated sklearn/metrics/tests/test_ranking.py Outdated
@Alexander-N

This comment has been minimized.

Show comment
Hide comment
@Alexander-N

Alexander-N Feb 25, 2018

Contributor

Great comments, thanks! Except returning 0 when max_fpr is 0, everything done as suggested.

Contributor

Alexander-N commented Feb 25, 2018

Great comments, thanks! Except returning 0 when max_fpr is 0, everything done as suggested.

@albertcthomas

I would have returned 0 when max_fpr is 0, even if it does not make sense, that's what the output should be in theory but LGTM. Thanks @Alexander-N!

@jnothman

Nice work. LGTM

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Feb 25, 2018

Member

Please add an entry to the change log at doc/whats_new/v0.20.rst. Like the other entries there, please reference this pull request with :issue: and credit yourself (and other contributors if applicable) with :user:

Member

jnothman commented Feb 25, 2018

Please add an entry to the change log at doc/whats_new/v0.20.rst. Like the other entries there, please reference this pull request with :issue: and credit yourself (and other contributors if applicable) with :user:

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Feb 25, 2018

Member

max_fpr=0 might have a theoretically value, but it is a nonsense setting for an evaluation metric. Error is good.

Member

jnothman commented Feb 25, 2018

max_fpr=0 might have a theoretically value, but it is a nonsense setting for an evaluation metric. Error is good.

@jnothman jnothman changed the title from [MRG] Partial AUC to [MRG+1] Partial AUC Feb 25, 2018

@albertcthomas

This comment has been minimized.

Show comment
Hide comment
@albertcthomas

albertcthomas Feb 25, 2018

Contributor

Fair enough.

Contributor

albertcthomas commented Feb 25, 2018

Fair enough.

@Alexander-N

This comment has been minimized.

Show comment
Hide comment
@Alexander-N

Alexander-N Feb 26, 2018

Contributor

Ok, done.

Contributor

Alexander-N commented Feb 26, 2018

Ok, done.

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Feb 26, 2018

Member

Please add cmmots rather than amending to make it easier to review the changes

Member

jnothman commented Feb 26, 2018

Please add cmmots rather than amending to make it easier to review the changes

@Alexander-N

This comment has been minimized.

Show comment
Hide comment
@Alexander-N

Alexander-N Feb 26, 2018

Contributor

Ah ok sorry. I amended to correct the original commit message. Only change is the addition of the entry in doc/whats_new/v0.20.rst.

Contributor

Alexander-N commented Feb 26, 2018

Ah ok sorry. I amended to correct the original commit message. Only change is the addition of the entry in doc/whats_new/v0.20.rst.

@jnothman

I was going to merge this, but I think it deserves a mention in doc/modules/model_evaluation.rather when discussing the auc score. Try to help users understand why this may be a better/worse metric for their task.

Add partial AUC to `roc_auc_score`
partial AUC: Integrate the ROC curve until chosen maximal FPR and
standardize the result to be 0.5 if non-discriminant and 1 if maximal.

Closes #3273
@Alexander-N

This comment has been minimized.

Show comment
Hide comment
@Alexander-N

Alexander-N Feb 28, 2018

Contributor

I could add something along the lines of

In applications where a high false positive rate is not tolerable the parameter ``max_fpr`` can be used to summarize the ROC curve up to the given limit.

I feel that this might not be very helpful. Do you have something specific in mind?

Contributor

Alexander-N commented Feb 28, 2018

I could add something along the lines of

In applications where a high false positive rate is not tolerable the parameter ``max_fpr`` can be used to summarize the ROC curve up to the given limit.

I feel that this might not be very helpful. Do you have something specific in mind?

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Feb 28, 2018

Member
Member

jnothman commented Feb 28, 2018

@Alexander-N

This comment has been minimized.

Show comment
Hide comment
@Alexander-N

Alexander-N Mar 1, 2018

Contributor

Ok, done.

Contributor

Alexander-N commented Mar 1, 2018

Ok, done.

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Mar 1, 2018

Member

I had been suggesting you put "one critique" first as a way to improve cohesion. Now it reads a bit strangely. (And I didn't really like my own wording here)

Perhaps you should just drop my words and leave your own.

Member

jnothman commented on doc/modules/model_evaluation.rst in c6a38d9 Mar 1, 2018

I had been suggesting you put "one critique" first as a way to improve cohesion. Now it reads a bit strangely. (And I didn't really like my own wording here)

Perhaps you should just drop my words and leave your own.

This comment has been minimized.

Show comment
Hide comment
@Alexander-N

Alexander-N Mar 1, 2018

Contributor

Ok, Done :-)

Contributor

Alexander-N replied Mar 1, 2018

Ok, Done :-)

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Mar 1, 2018

Member
Member

jnothman replied Mar 1, 2018

@jnothman

Thanks

@jnothman jnothman merged commit 02ddc70 into scikit-learn:master Mar 1, 2018

4 of 6 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
lgtm analysis: Python Running analyses for revisions
Details
ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: python2 Your tests passed on CircleCI!
Details
ci/circleci: python3 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment