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

ENH Adds plot_det_curve and associated display #18176

Merged
merged 22 commits into from Aug 29, 2020

Conversation

glemaitre
Copy link
Contributor

@glemaitre glemaitre commented Aug 17, 2020

Add the plotting helpers for the DET curve

Follow-up to #18169
Closes #18181

@glemaitre glemaitre marked this pull request as draft August 17, 2020 09:07
@glemaitre glemaitre marked this pull request as ready for review August 19, 2020 12:13
@glemaitre
Copy link
Contributor Author

ping @thomasjpfan @lorentzenchr @agramfort

I renamed the function and added the plotting utilities.

Copy link
Member

@lorentzenchr lorentzenchr left a comment

Choose a reason for hiding this comment

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

Overall it looks very good. I've two open questions:

  1. What about adding an optional argument to switch off/on the Normal transformed scale (call to sp.stats.norm.ppf) ?
  2. The tests are mostly copy&past from tests of plot_roc_curve. Could we elegantly avoid this duplication?

sklearn/metrics/_plot/det_curve.py Outdated Show resolved Hide resolved
sklearn/metrics/_plot/det_curve.py Outdated Show resolved Hide resolved
sklearn/metrics/_plot/det_curve.py Outdated Show resolved Hide resolved
sklearn/metrics/_plot/det_curve.py Outdated Show resolved Hide resolved
sklearn/metrics/_plot/det_curve.py Outdated Show resolved Hide resolved
sklearn/metrics/_plot/det_curve.py Outdated Show resolved Hide resolved
sklearn/metrics/_plot/tests/test_plot_det_curve.py Outdated Show resolved Hide resolved
sklearn/metrics/_plot/tests/test_plot_det_curve.py Outdated Show resolved Hide resolved
examples/model_selection/plot_det.py Outdated Show resolved Hide resolved
glemaitre and others added 9 commits August 21, 2020 17:30
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
@glemaitre
Copy link
Contributor Author

What about adding an optional argument to switch off/on the Normal transformed scale (call to sp.stats.norm.ppf) ?

I think this is one of the properties of the DET curve. I am not sure that we should propose a parameter. We could be conservative at first releasing without his feature and see if there is a need for it.

@glemaitre
Copy link
Contributor Author

The tests are mostly copy&past from tests of plot_roc_curve. Could we elegantly avoid this duplication?

This should be possible :)

@glemaitre
Copy link
Contributor Author

Only the test specific to each curve is not tested commonly because it might be easier to discover and understand what is happening at the cost of being slightly redundant.

@glemaitre glemaitre added this to WAITING FOR REVIEW in Guillaume's pet Aug 24, 2020
sklearn/metrics/_plot/det_curve.py Outdated Show resolved Hide resolved
if "label" in line_kwargs:
ax.legend(loc="lower right")

ticks = [0.001, 0.01, 0.05, 0.20, 0.5, 0.80, 0.95, 0.99, 0.999]
Copy link
Member

Choose a reason for hiding this comment

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

Should these ticks be user adjustable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would say this is a good default. Then we return ax so the user can customize it.

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM

@thomasjpfan thomasjpfan changed the title ENH add plot_det_curve and associated display ENH Adds plot_det_curve and associated display Aug 29, 2020
@thomasjpfan thomasjpfan merged commit e030010 into scikit-learn:master Aug 29, 2020
5 checks passed
@glemaitre glemaitre moved this from WAITING FOR REVIEW to WAITING FOR CONSENSUS in Guillaume's pet Aug 31, 2020
@glemaitre glemaitre moved this from WAITING FOR CONSENSUS to MERGED in Guillaume's pet Aug 31, 2020
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

RFC rename detection_error_tradeoff_curve det_curve
3 participants