Skip to content

DOC Add reference to CalibrationDisplay from calibration_curve's docstring #31312

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 5 commits into from
May 6, 2025

Conversation

metlouf
Copy link
Contributor

@metlouf metlouf commented May 5, 2025

Reference Issues/PRs

Fixes #31311. See also #31302
Reference CalibrationDisplay from calibration_curve's docstring in a "See also section"

What does this implement/fix? Explain your changes.

Modify Doc of CalibrationDisplay

Any other comments?

Copy link

github-actions bot commented May 5, 2025

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: ef5f13a. Link to the linter CI: here

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.

Thanks for the PR. I checked the rendered HTML doc linked from the CI report of this PR (Check the rendered docs here!, reachable by expanding the "All checks have passed" menu) and the results look good:

image

Maybe we could be a little bit more descriptive as suggested below, but otherwise LGTM.

@@ -1005,6 +1005,10 @@ def calibration_curve(
prob_pred : ndarray of shape (n_bins,) or smaller
The mean predicted probability in each bin.

See Also
--------
sklearn.calibration.CalibrationDisplay : Calibration curve visualization.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sklearn.calibration.CalibrationDisplay : Calibration curve visualization.
sklearn.calibration.CalibrationDisplay : Calibration curve visualization
based on matplotlib.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, rather than linking to the class itself, I think we should directly link to the sklearn.calibration.CalibrationDisplay.from_estimator and from_predictions factory methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I modified as suggested, I copied the sklearn.calibration.CalibrationDisplay.from_estimator and from_predictions description from another function for consistency, waiting for test to get approved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
Here it is !

Copy link
Member

@ArturoAmorQ ArturoAmorQ left a comment

Choose a reason for hiding this comment

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

Thanks @metlouf and congrats on your first merge in scikit-learn!

@ArturoAmorQ ArturoAmorQ changed the title Reference CalibrationDisplay from calibration_curve's docstring in a "See also section" DOC Add reference to CalibrationDisplay from calibration_curve's docstring May 6, 2025
@ArturoAmorQ ArturoAmorQ merged commit 7a88bf1 into scikit-learn:main May 6, 2025
37 checks passed
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.

Reference CalibrationDisplay from calibration_curve's docstring in a "See also section"
3 participants