FIX Ensure that index is correct when global transform_output is pandas #26454
Conversation
| @@ -627,7 +627,7 @@ def _initial_imputation(self, X, in_fit=False): | |||
| strategy=self.initial_strategy, | |||
| fill_value=self.fill_value, | |||
| keep_empty_features=self.keep_empty_features, | |||
| ) | |||
| ).set_output(transform="default") | |||
There was a problem hiding this comment.
I don't love this, cause it seems it could be quite buggy, as in, in our codebase I don't know where else we have such instances where we're not explicitly setting the output type.
I would suggest having a default which is to raise and be like: you haven't explicitly set this value for this estimator, please do so, and have that mode for scikit-learn at least.
This way we'd know we should always set it explicitly to: default, pandas, or the global config.
WDYT?
There was a problem hiding this comment.
I can get behind a new "raise_if_not_configured" configuration flag. Implementation-wise we'll need a standard place to check that configuration at fit. That can be in _validate_data or _fit_context. _fit_context is a more natural place but it is not used everywhere yet.
There was a problem hiding this comment.
_fit_context is a more natural place but it is not used everywhere yet.
It will be the case when #26473 is merged
|
Even with the suggestion in #26454 (comment), we'll end up with the same solution as this PR. The suggestion would improve our coverage for catching this type of bug. |
Reference Issues/PRs
Fixes #26443
What does this implement/fix? Explain your changes.
This PR updates the common test to ensure that the output dataframe from
transformis consistent with the input. I updatedIsomap,IterativeImputerandPowerTransformerso they pass the updated common test.