Skip to content

API add from_estimator and from_predictions to RocCurveDisplay#20569

Merged
ogrisel merged 21 commits intoscikit-learn:mainfrom
glemaitre:api_RocCurveDisplay
Sep 3, 2021
Merged

API add from_estimator and from_predictions to RocCurveDisplay#20569
ogrisel merged 21 commits intoscikit-learn:mainfrom
glemaitre:api_RocCurveDisplay

Conversation

@glemaitre
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

glemaitre commented Jul 25, 2021

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

@ogrisel ogrisel Sep 3, 2021

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.

@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