Skip to content

ENH follow-up style improvements of DET curves #10591 #18169

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

Merged
merged 9 commits into from
Aug 19, 2020

Conversation

glemaitre
Copy link
Member

@glemaitre glemaitre commented Aug 16, 2020

Follow-up of #10591.
Closes #18174.

@glemaitre glemaitre changed the title ENH follow-up style improvement 10591 ENH follow-up style improvement #10591 Aug 16, 2020
@glemaitre
Copy link
Member Author

ping @cmarmo @agramfort @jnothman who reviewed #10591

@glemaitre
Copy link
Member Author

We should open an issue to add the plot_* function as well.

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

nits but lgtm, thanks @glemaitre

@@ -1,7 +1,7 @@
"""
=======================================
====================================
Copy link
Member

Choose a reason for hiding this comment

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

should this be in metric instead of model_selection ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The plot_roc is also in this section. I am not sure what is best. I think that the idea is to compare model and select the best one in some sense. So it is not so wrong to put it in this section.

@NicolasHug
Copy link
Member

There's also the "False Negative Rate" ylabel that overlaps with the left plot in the example.

@glemaitre
Copy link
Member Author

ping @lorentzenchr @rth as well :P

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

forgot to approve

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

forgot to approve

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.

Don't know how to add a review comment to the following:

  • In the docstring of detection_error_tradeoff_curve, replace "Note: This metrics is used for ranking evaluation of a binary" by "This metric is used for evaluation of ranking and error tradeoffs of a binary".
  • In the User Guide, can we replace the word "ordinate" by "y-axis"?

@glemaitre
Copy link
Member Author

@lorentzenchr Done

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.

nitpick, otherwise LGTM

Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
@glemaitre
Copy link
Member Author

Good to be merged :)

@lorentzenchr lorentzenchr changed the title ENH follow-up style improvement #10591 ENH follow-up style improvements of DET curves #10591 Aug 19, 2020
@lorentzenchr lorentzenchr merged commit bf4714f into scikit-learn:master Aug 19, 2020
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
…ikit-learn#18169)

* follow-up on scikit-learn#10591 

* DOC improvements

* TST add additional test for pos_label

Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
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.

Fix docstring of detection_error_tradeoff_curve
3 participants