FEA metadata routing for StackingClassifier and StackingRegressor#28701
FEA metadata routing for StackingClassifier and StackingRegressor#28701adrinjalali merged 21 commits intoscikit-learn:mainfrom
StackingClassifier and StackingRegressor#28701Conversation
| .. deprecated:: 1.5 | ||
| `sample_weight` is deprecated in 1.5 and will be removed in 1.7. |
There was a problem hiding this comment.
in this class it's not deprecated though, it's simply removed.
| def fit_transform(self, X, y, sample_weight=None): | ||
| def fit_transform(self, X, y, sample_weight=None, **fit_params): |
There was a problem hiding this comment.
I think we should deprecate positional sample_weight here as well.
There was a problem hiding this comment.
Yes, we should. Otherwise a sample_weight passed as a fit_param into the routing would mistakenly be passed through the old way outside of the routing.
| record_metadata_not_default( | ||
| self, "predict_proba", sample_weight=sample_weight, metadata=metadata | ||
| ) | ||
| return np.asarray([[0.0, 1.0]] * len(X)) |
There was a problem hiding this comment.
In StackingClassifier, there's a stack_method param, whiches value can be set to predict_proba as well. In StackingRregessor it's just hardcoded as predict.
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
StefanieSenger
left a comment
StefanieSenger
left a comment
There was a problem hiding this comment.
@adrinjalali
Thanks for reviewing. I went through it and improved regarding your comments.
| .. deprecated:: 1.5 | ||
| `sample_weight` is deprecated in 1.5 and will be removed in 1.7. |
| def fit_transform(self, X, y, sample_weight=None): | ||
| def fit_transform(self, X, y, sample_weight=None, **fit_params): |
There was a problem hiding this comment.
Yes, we should. Otherwise a sample_weight passed as a fit_param into the routing would mistakenly be passed through the old way outside of the routing.
| record_metadata_not_default( | ||
| self, "predict_proba", sample_weight=sample_weight, metadata=metadata | ||
| ) | ||
| return np.asarray([[0.0, 1.0]] * len(X)) |
There was a problem hiding this comment.
In StackingClassifier, there's a stack_method param, whiches value can be set to predict_proba as well. In StackingRregessor it's just hardcoded as predict.
|
So, the RecursionError bug that is about to be fixed in #28712 is also appearing here. |
|
@StefanieSenger since #28712 is merged, wanna merge with |
|
I have merged main into it and all tests now pass, @adrinjalali |
OmarManzoor
left a comment
OmarManzoor
left a comment
There was a problem hiding this comment.
Thanks for the PR @StefanieSenger. Just a few minor comments otherwise this looks good.
Co-authored-by: Omar Salman <omar.salman@arbisoft.com>
StefanieSenger
left a comment
StefanieSenger
left a comment
There was a problem hiding this comment.
Thanks for reviewing @OmarManzoor. I have committed those changes. :)
OmarManzoor
left a comment
OmarManzoor
left a comment
There was a problem hiding this comment.
A further suggestion
Co-authored-by: Omar Salman <omar.salman@arbisoft.com>
Oh, that's wonderful, thank you, @OmarManzoor :) |
OmarManzoor
left a comment
OmarManzoor
left a comment
There was a problem hiding this comment.
LGTM. Thanks @StefanieSenger. The versions will just need to be changed to 1.6 now that the branch for 1.5 has already been separated.
| transformers' ``fit`` and ``fit_transform``. :pr:`28205` by :user:`Stefanie | ||
| Senger <StefanieSenger>`. | ||
|
|
||
| - |Feature| :class:`ensemble.StackingClassifier` and |
There was a problem hiding this comment.
I think this might need to move to 1.6 now.
Co-authored-by: Omar Salman <omar.salman@arbisoft.com>
3 participants
Reference Issues/PRs
towards #22893
closes #18028
What does this implement/fix? Explain your changes.
Adds metadata routing to
StackingClassifierandStackingRegressor.Any other comments?
I wasn't sure if we want to route metadata within the predict method. It's already implemented in
_BaseStacking.predict(self, X, **predict_params), but without needing to set a request for it. What is the preferable way here?Also there is an issue whenever
RidgeCV(the default) is thefinal_estimator: We get aRecursionError, because it gets hung in_metadata_requests.py. I will continue to invest about this. Update: This is a bug in the routing mechanism ofRidgeCVand it's unrelated to this PR. I've opened a PR to fix it: FIXRecursionErrorbug with metadata routing in metaestimators with scoring #28712@adrinjalali @OmarManzoor @glemaitre, do you want to have a look?