-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
API Rename base_estimator in CalibratedClassifierCV #22054
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
Conversation
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.
We should as well in the calibration.rst
and example that we don't have any occurrence of base_estimator
there.
Hello @kevroi, are you still interested in working on PR? If so, there are review comments to be addressed from #22054 (review) |
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.
LGTM.
@thomasjpfan do you want to have a quick pass.
sklearn/calibration.py
Outdated
"will be removed in 1.4.", | ||
FutureWarning, | ||
) | ||
self.estimator = self.base_estimator |
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.
This is usually an anti-pattern in scikit-learn. In this case, I think we we can get away with:
if self.base_estimator != "deprecated":
...
estimator = self.base_estimator
else:
estimator = self.estimator
# Estimator from now on
if estimator is None:
estimator = LinearSVC(random_state=0)
if self.cv == "prefit":
check_is_fitted(estimator, ...)
WDYT @glemaitre ?
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 see what you mean. I think that you are right. Let me update it.
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.
LGTM
Co-authored-by: Kevin Roice <kevinroice@Kevins-Air.broadband> Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Reference Issues/PRs
partly addresses #9104
What does this implement/fix? Explain your changes.
Renames
base_estimator
toestimators
for the CalibratedClassifierCV byrenaming in the
CalibratedClassifierCV
and__CalibratedClassifier
classes