Skip to content

API add from_estimator and from_preditions to PrecisionRecallDisplay #20552

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

Conversation

glemaitre
Copy link
Member

@glemaitre glemaitre commented Jul 17, 2021

Address some of #15880

Add the class methods from_estimator and from_predictions to the class PrecisionRecallDisplay. In addition, we deprecate the plot_precision_recall_curve in favour of these two class methods.

TODO:

  • Add new tests for the two methods
  • Add a new test to check the deprecation
  • Update the user guide
  • Update examples

@glemaitre glemaitre marked this pull request as draft July 17, 2021 22:16
@glemaitre glemaitre marked this pull request as ready for review July 19, 2021 15:36
@glemaitre glemaitre changed the title ENH add from_estimator and from_preditions to PredictionRecallDisplay API add from_estimator and from_preditions to PredictionRecallDisplay Jul 19, 2021
@@ -0,0 +1,138 @@
import pytest
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to reviewer: this test will be used as well for the RocCurveDisplay and the DetCurveDisplay

@glemaitre glemaitre changed the title API add from_estimator and from_preditions to PredictionRecallDisplay API add from_estimator and from_preditions to PrecisionRecallDisplay Jul 19, 2021
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.

Thank you for working on this @glemaitre !

Few comments, otherwise LGTM.

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

Thanks! Could you please resolve merge conflicts? I'll review tests when that is done..

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

Generally LGTM. Not sure why codecov doesn't show in CI, but the report is in https://app.codecov.io/gh/scikit-learn/scikit-learn/compare/20552

@@ -144,7 +152,206 @@ def plot(self, ax=None, *, name=None, **kwargs):
self.figure_ = ax.figure
return self
Copy link
Member

Choose a reason for hiding this comment

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

I still find it counter-intuitive that plot returns self instead of ax, but that's unrelated to this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't really recall if there is any reason. Maybe it allows pipelining some operations but it is true that pandas/seaborn usually return an axis

@glemaitre glemaitre added this to the 1.0 milestone Aug 30, 2021
@glemaitre
Copy link
Member Author

There is 2 approvals here to I will merge this one to fix the conflict in #20569

@glemaitre glemaitre merged commit d4da690 into scikit-learn:main Aug 31, 2021
@glemaitre glemaitre removed the Blocker label Aug 31, 2021
samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
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.

3 participants