8000 API add from_estimator and from_predictions to RocCurveDisplay by glemaitre · Pull Request #20569 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

API add from_estimator and from_predictions to RocCurveDisplay #20569

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.

Sign up for GitHub

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
Merged
merged 21 commits into from
Sep 3, 2021
Merged

API add from_estimator and from_predictions to RocCurveDisplay #20569

merged 21 commits into from
Sep 3, 2021

Conversation

glemaitre
Copy link
Member
@glemaitre glemaitre commented Jul 19, 2021 •
edited
Loading

Address some of #15880

Add from_estimator and from_predictions method and deprecate the plot_roc_curve.

TODO:

  • Add new class methods
  • Deprecate plot_roc_curve
  • Add check for raising deprecation warnings
  • Catch deprecation warning in test
  • Add common test for the curve
  • Add tests for the display
  • Update the examples
  • Update the user guide

@glemaitre glemaitre mentioned this pull request Jul 19, 2021
Closed
5 tasks
@glemaitre glemaitre marked this pull request as draft July 19, 2021 21:42
@glemaitre glemaitre marked this pull request as ready for review July 20, 2021 08:02
thomasjpfan
thomasjpfan reviewed Jul 24, 2021
View reviewed changes
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.

Overall looks good.

How should we handle the release hightlights for 0.22?

scikit-learn/examples/release_highlights/plot_release_highlights_0_22_0.py

Lines 51 to 53 in 3d3d176

svc_disp = plot_roc_curve(svc, X_test, y_test)
rfc_disp = plot_roc_curve(rfc, X_test, y_test, ax=svc_disp.ax_)
rfc_disp.figure_.suptitle("ROC curve comparison")

I guess we can comment out the deprecated code, use from_estimator and add a comment?

@glemaitre
Copy link
Member Author

I guess we can comment out the deprecated code, use from_estimator and add a comment?

I would delay the change for when we will remove the function. But I assume that commenting will be the only way to manage these issues.

thomasjpfan
thomasjpfan approved these changes Aug 29, 2021
View reviewed changes
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.

Small comment, otherwise LGTM!

@glemaitre @thomasjpfan
Apply suggestions from code review
d4c11c8 
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
@glemaitre glemaitre modified the milestones: 1.1, 1.0 Aug 30, 2021
@glemaitre glemaitre mentioned this pull request Aug 31, 2021
Merged
4 tasks
adrinjalali
adrinjalali reviewed Sep 2, 2021
View reviewed changes
Copy link
Member
@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

@scikit-learn/core-devs this needs another review if it's going to be in the release. I'd like to call a feature freeze on Monday.

@glemaitre @adrinjalali
Update doc/visualizations.rst
6901be2 
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
ogrisel
ogrisel approved these changes Sep 3, 2021
View reviewed changes
Copy link
Member
@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

Looks very good. Thanks @glemaitre!

sklearn/metrics/_plot/tests/test_roc_curve_display.py
@pytest.fixture(scope="module")
def data_binary(data):
X, y = data
return X[y < 2], y[y < 2]
Copy link
Member

Choose a reason for hiding this comment

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

Very nice use of module-scoped fixtures.

8000
@glemaitre @ogrisel
Update doc/developers/plotting.rst
dcf0805 
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
@ogrisel ogrisel merged commit f461908 into scikit-learn:main Sep 3, 2021
adrinjalali added a commit to adrinjalali/scikit-learn that referenced this pull request Sep 5, 2021
@glemaitre @thomasjpfan
@adrinjalali @ogrisel
API add from_estimator and from_predictions to RocCurveDisplay (sciki…
c206acf 
…t-learn#20569)

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
adrinjalali added a commit to adrinjalali/scikit-learn that referenced this pull request Sep 5, 2021
@glemaitre @thomasjpfan
@adrinjalali @ogrisel
API add from_estimator and from_predictions to RocCurveDisplay (sciki…
abbc21a 
…t-learn#20569)

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
adrinjalali added a commit that referenced this pull request Sep 6, 2021
@glemaitre @thomasjpfan
@adrinjalali @ogrisel
API add from_estimator and from_predictions to RocCurveDisplay (#20569)
22cb102 
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
@glemaitre @thomasjpfan
@adrinjalali @ogrisel @samronsin
API add from_estimator and from_predictions to RocCurveDisplay (sciki…
df49a61 
…t-learn#20569)

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrinjalali adrinjalali adrinjalali left review comments

@ogrisel ogrisel ogrisel approved these changes

@thomasjpfan thomasjpfan thomasjpfan approved these changes

Assignees
No one assigned
Labels
Blocker module:metrics
Projects
None yet
Milestone
1.0
Development

Successfully merging this pull request may close these issues.
4 participants
@glemaitre @ogrisel @adrinjalali @thomasjpfan
0