REVERT ENH add the parameter prefit in the FixedThresholdClassifier (#29067)#30172
Conversation
|
Uhm can you make a push with |
glemaitre
left a comment
glemaitre
left a comment
There was a problem hiding this comment.
Everything is green so LGTM apart from the last comment.
thomasjpfan
left a comment
thomasjpfan
left a comment
There was a problem hiding this comment.
I do not think it's obvious for someone to infer all the features that FrozenEstimator enables. (Including using it as a pre-fitted estimator in FixedThresholdClassifier)
Can we have a central place in the docs that describe some of the features FrozenEstimator enables?
|
Added an example here. If we merge this one first, I can add to the same example for |
| model_fixed_threshold = FixedThresholdClassifier( | ||
| estimator=model, threshold=tuned_model.best_threshold_, prefit=True | ||
| estimator=FrozenEstimator(model), threshold=tuned_model.best_threshold_ | ||
| ).fit(data_train, target_train) |
There was a problem hiding this comment.
It's a bit weird that .fit(data_train, target_train) is still needed in this case (just to set the internal estimator_ attribute).
Maybe FixedThresholdClassifier.predict could be changed to avoid calling check_is_fitted(self, "estimator_") and use:
estimator = getattr(self, "estimator_", self.estimator)
...Then the call to _get_response_values_binary would naturally call the check_is_fitted of the wrapped estimator to make sure that we raise the excepted exception if the underlying estimator has not been fitted beforehand.
This would allow using it with a FrozenEstimator without the useless call to .fit.
There was a problem hiding this comment.
I don't think it's odd. It's expected. Fit doesn't just do fit in so many cases. So I would have kept it as is.
But my last commit now does this (it's slightly bigger than the change you suggest).
Note that this means technically, the user doesn't have to wrap the sub-estimator in a FrozenEstimator.
There was a problem hiding this comment.
But my last commit now does this (it's slightly bigger than the change you suggest).
I don't see the commit you are referring to. Did you push it to an alternative branch?
ogrisel
left a comment
ogrisel
left a comment
There was a problem hiding this comment.
Let merge and iterate in further PRs if needed.
| # | ||
| # Please refer to | ||
| # :ref:`sphx_glr_auto_examples_model_selection_plot_cost_sensitive_learning.py` | ||
| # to learn about cost-sensitive learning and decision threshold tuning. |
There was a problem hiding this comment.
I cannot check the HTML rendering of this new example on the CI:
From:
|
Actually, I cannot click the merge button:
|
|
Me too. It looks like a GitHub bug. The branch is "frozen". |
|
It was a github issue. Now it's working. I also added to the example the |
|
Now we can merge it apparently since @adrinjalali pushed some fixes since. |
4 participants
Towards #29893
This reverts #29067, and instead adds a note that users can use
FrozenEstimator.This doesn't need a deprecation since we haven't released it yet.
cc @glemaitre @ogrisel