-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
Fix calibration_curve docstring for empty bins #12926
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
Fix calibration_curve docstring for empty bins #12926
Conversation
The code for calibration_curve will remove bins that are empty, thus the number of bins in the return value may be smaller than n_bins. This commit fixes the docstring to document this (existing) behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
sklearn/calibration.py
Outdated
|
||
Returns | ||
------- | ||
prob_true : array, shape (n_bins,) | ||
The true probability in each bin (fraction of positives). | ||
prob_true : array, shape (n_non_empty_bins,) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this should be expressed as (n_bins,) or smaller
to avoid confusion. or (n_bins,)
with a comment below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think (n_bins,) or smaller
would work well. If we just had (n_bins,)
on the first line, however, I'd worry that it would be easy to miss the caveat/comment below. A footnote would be another option, but your first suggestion seems clearest to me. Should I modify the pull request?
Yes, please add a commit
|
Changed `shape (n_non_empty_bins)` to `shape (n_bins,) or smaller` based on reviewer feedback of pull request.
…ing (scikit-learn#12926)" This reverts commit f1e78a6.
…ing (scikit-learn#12926)" This reverts commit f1e78a6.
The current docstring for calibration.calibration_curve states that the return values are of shape (n_bins,) The code for calibration_curve however, will remove bins that are empty (which is not mentioned in the current docstring), thus the number of bins in the return value may be smaller than n_bins. This commit fixes the docstring to document the (existing) behavior of removing empty bins.
What does this implement/fix? Explain your changes.
When a bin is empty, calibration_curve will remove it from the list of bins, resulting in return values with less than n_bins:
The existing documentation for this function makes no mention of removing empty bins, however, and states that the return values with be of shape (n_bins,):
Obviously it's best of the documentation matches the behavior of the code, and changing the behavior of the code would risk breaking existing uses of the code. This check in thus updates the documentation to include the behavior of dropping empty bins.
Any other comments?
I encountered this in with real data when trying to generate confidence intervals for a calibration curve using cross validation. The model used weakly correlating features and thus high predicted probabilities were rare and not present in some folds. This lead to differing numbers of bins in each fold causing errors downstream, and the undocumented behavior slowed down the debugging process.
It some cases (such as mine) it would be better to support the behavior of the original documentation (always returning the requested number of bins, even if some are empty) by returning NaNs for empty bins which the end user can then handle appropriately. This could be safely added with a parameter such as
drop_empty
which could default toTrue
for compatibility. I would be happy to create the pull request if there is interest, but I am assuming there is not enough interest to justify maintaining and testing another control path in the code.