[MRG] BUG fix inconsistency between fit and fit_transform in FeatureUnion #10098
Conversation
|
Is this the right regression test? Shouldn't we use the tuple test from #9716 on |
| X = np.array([[1, 2]]) | ||
| pipeline_estimators = {'transf': Transf(), | ||
| 'clf': FitParamT()} | ||
| pipe = Pipeline(pipeline_estimators.items()) |
There was a problem hiding this comment.
what does that test? That we allow items in pipelines? Do we want to guarantee that?
|
Since we put a featureunion in the pipeline, fit transform is called under the hood. The tests that I wrote is to solve the dict items failure but his is true that we do not accept them explicitly so probably this is not what we should test.
|
|
ok let's support dict items |
|
Uhm I did not even remember this one ;) |
|
Can we really support this in Pipeline where order matters, |
|
@jnothman but in older Pythons the order is not deterministic, right? I'm probably missing what you're suggesting. |
|
Yes, that's what I was trying to say. Supportin dictitems in Pipeline is problematic |
|
Ok but no-one is proposing that and this PR is not doing that, right? |
|
This PR was actually to remove the inconsistency between Shall we support For sure, I should remove this test. |
|
Solved in master after some code refactoring |
Reference Issues/PRs
closes #10088
What does this implement/fix? Explain your changes.
Solve the inconsistent behaviour between
fitandfit_transformin FeatureUnion.Add test to check that either Pipeline and FeatureUnion accept dict.items().
Any other comments?