The Wayback Machine - https://web.archive.org/web/20210915173114/https://github.com/scikit-learn/scikit-learn/pull/18176
Skip to content

/ scikit-learn Public

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

ENH Adds plot_det_curve and associated display #18176

Merged
merged 22 commits into from Aug 29, 2020
Merged

ENH Adds plot_det_curve and associated display #18176

merged 22 commits into from Aug 29, 2020

Conversation

@glemaitre
Copy link
Contributor

@glemaitre glemaitre commented Aug 17, 2020 •
edited

Add the plotting helpers for the DET curve

Follow-up to #18169
Closes #18181
@glemaitre
ENH add plot_det_curve and associated display
726b2c7
@glemaitre glemaitre marked this pull request as draft Aug 17, 2020
@glemaitre
TST add basic tests
41f27e0
@glemaitre glemaitre mentioned this pull request Aug 17, 2020
Merged
6 tasks
glemaitre added 5 commits Aug 17, 2020
@glemaitre
iter
518c7e3
@glemaitre
PEP8
50100f4
@glemaitre
Merge remote-tracking branch 'origin/master' into add_plot_det
84c43d4
@glemaitre
rename function and update plots
bf00428
@glemaitre
remaining fix
8554321
@glemaitre glemaitre marked this pull request as ready for review Aug 19, 2020
@glemaitre
Copy link
Contributor Author

@glemaitre glemaitre commented Aug 19, 2020

ping @thomasjpfan @lorentzenchr @agramfort

I renamed the function and added the plotting utilities.

@glemaitre
fix
69050e6
lorentzenchr
lorentzenchr reviewed Aug 21, 2020
View changes
Copy link
Member

@lorentzenchr lorentzenchr left a comment

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 Aug 21, 2020
@glemaitre @lorentzenchr
Update sklearn/metrics/_plot/det_curve.py
fb8ddaa 
Co-authored-by: Christian Lorentzen <[email protected]>
@glemaitre @lorentzenchr
Update sklearn/metrics/_plot/det_curve.py
cbaf03f 
Co-authored-by: Christian Lorentzen <[email protected]>
@glemaitre @lorentzenchr
Update sklearn/metrics/_plot/det_curve.py
857ec3f 
Co-authored-by: Christian Lorentzen <[email protected]>
@glemaitre @lorentzenchr
Update sklearn/metrics/_plot/det_curve.py
13537d0 
Co-authored-by: Christian Lorentzen <[email protected]>
@glemaitre @lorentzenchr
Update sklearn/metrics/_plot/det_curve.py
5df7a54 
Co-authored-by: Christian Lorentzen <[email protected]>
@glemaitre @lorentzenchr
Update sklearn/metrics/_plot/det_curve.py
844f01f 
Co-authored-by: Christian Lorentzen <[email protected]>
@glemaitre @lorentzenchr
Update sklearn/metrics/_plot/tests/test_plot_det_curve.py
9fe7114 
Co-authored-by: Christian Lorentzen <[email protected]>
@glemaitre @lorentzenchr
Update sklearn/metrics/_plot/tests/test_plot_det_curve.py
5020b96 
Co-authored-by: Christian Lorentzen <[email protected]>
@glemaitre
add examples
46c957b
@glemaitre
Copy link
Contributor Author

@glemaitre glemaitre commented Aug 21, 2020

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
Merge remote-tracking branch 'glemaitre/add_plot_det' into add_plot_det
69b986e
@glemaitre
Copy link
Contributor Author

@glemaitre glemaitre commented Aug 21, 2020

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

This should be possible :)

@glemaitre
TST common tests
aa431e2
@glemaitre
Copy link
Contributor Author

@glemaitre glemaitre commented Aug 21, 2020

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 added 2 commits Aug 21, 2020
@glemaitre
PEP8
88254a1
@glemaitre
PEP8
fc63d29
lorentzenchr
lorentzenchr approved these changes Aug 21, 2020
View changes
@glemaitre glemaitre added this to WAITING FOR REVIEW in Guillaume's pet Aug 24, 2020
thomasjpfan
thomasjpfan reviewed Aug 25, 2020
View changes
sklearn/metrics/_plot/det_curve.py Outdated Show resolved Hide resolved
sklearn/metrics/_plot/det_curve.py
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

@thomasjpfan thomasjpfan Aug 25, 2020

Should these ticks be user adjustable?

Copy link
Contributor Author

@glemaitre glemaitre Aug 26, 2020

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

@glemaitre @thomasjpfan
Apply suggestions from code review
cf2b2d4 
Co-authored-by: Thomas J. Fan <[email protected]>
thomasjpfan
thomasjpfan approved these changes Aug 29, 2020
View changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

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
19 checks passed
19 checks passed
@lgtm-com[bot]
LGTM analysis: C/C++ No code changes detected
Details
@lgtm-com[bot]
LGTM analysis: JavaScript No code changes detected
Details
@lgtm-com[bot]
LGTM analysis: Python No new or fixed alerts
Details
ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: doc Your tests passed on CircleCI!
Details
@circleci-artifacts-redirector[bot]
ci/circleci: doc artifact Link to 0/doc/_changed.html
Details
ci/circleci: doc-min-dependencies Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
@azure-pipelines[bot]
scikit-learn.scikit-learn Build #20200826.20 succeeded
Details
@azure-pipelines[bot]
scikit-learn.scikit-learn (Linting) Linting succeeded
Details
@azure-pipelines[bot]
scikit-learn.scikit-learn (Linux py36_conda_openblas) Linux py36_conda_openblas succeeded
Details
@azure-pipelines[bot]
scikit-learn.scikit-learn (Linux py36_ubuntu_atlas) Linux py36_ubuntu_atlas succeeded
Details
@azure-pipelines[bot]
scikit-learn.scikit-learn (Linux pylatest_pip_openblas_pandas) Linux pylatest_pip_openblas_pandas succeeded
Details
@azure-pipelines[bot]
scikit-learn.scikit-learn (Linux32 py36_ubuntu_atlas_32bit) Linux32 py36_ubuntu_atlas_32bit succeeded
Details
@azure-pipelines[bot]
scikit-learn.scikit-learn (Linux_Runs pylatest_conda_mkl) Linux_Runs pylatest_conda_mkl succeeded
Details
@azure-pipelines[bot]
scikit-learn.scikit-learn (Windows py36_pip_openblas_32bit) Windows py36_pip_openblas_32bit succeeded
Details
@azure-pipelines[bot]
scikit-learn.scikit-learn (Windows py37_conda_mkl) Windows py37_conda_mkl succeeded
Details
@azure-pipelines[bot]
scikit-learn.scikit-learn (macOS pylatest_conda_mkl) macOS pylatest_conda_mkl succeeded
Details
@azure-pipelines[bot]
scikit-learn.scikit-learn (macOS pylatest_conda_mkl_no_openmp) macOS pylatest_conda_mkl_no_openmp succeeded
Details
@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 added a commit to jayzed82/scikit-learn that referenced this issue Oct 22, 2020
@glemaitre @lorentzenchr
@thomasjpfan @jayzed82
ENH Adds plot_det_curve and associated display (scikit-learn#18176)
1030ea8 
Co-authored-by: Christian Lorentzen <[email protected]>
Co-authored-by: Thomas J. Fan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasjpfan thomasjpfan

@lorentzenchr lorentzenchr

Assignees
No one assigned
Labels
module:metrics
Projects
Guillaume's pet
MERGED
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

RFC rename detection_error_tradeoff_curve det_curve
3 participants
@glemaitre @thomasjpfan @lorentzenchr