[MRG + 1] BUG: get_param in FeatureUnion disjoint for shallow/deep params #4467
Conversation
|
|
| @@ -377,3 +378,15 @@ def test_transformer_n_iter(): | |||
|
|
|||
| if hasattr(estimator, "max_iter") and name not in external_solver: | |||
| yield check_transformer_n_iter, name, estimator | |||
|
|
|||
| def test_get_params_invariance(): | |||
There was a problem hiding this comment.
Can you add this yield to the test_all_estimators please?
There was a problem hiding this comment.
Is this in addition to where it already is?
test_all_estimatorsdoesn't callall_estimatorswith theinclude_otherflag so it excludesPipelineandFeatureUnion.- right now the test isn't written to handle other metaestimators, so either I can change the test or it can go in
test_non_meta_estimators?
Thanks!
There was a problem hiding this comment.
Sorry, I overlooked that. We should try to unify tests as much as possible, but I guess for a regression test the current variant is fine.
There was a problem hiding this comment.
Doing include_other=True in test_all_estimators would be nice, and allowing the other estimators in this test, would be nice, too, but we can leave that for the future.
|
Thanks for the PR. Travis is unhappy. |
| pass | ||
|
|
||
| def fit(self, X, y): | ||
| pass |
There was a problem hiding this comment.
fit should probably return self, even if not strictly needed now.
|
have you tried investigating the travis error? |
|
It looks like I created a regression on line 90 of sklearn.tests.test_pipeline.test_pipeline_init. I'll look into it later today. Thanks. By your comment "We should try to unify tests as much as possible, but I guess for a regression test the current variant is fine," does that mean leaving the test as is is fine? (aside from fixing the regression, and adding |
|
Yeah, its fine to leave the tests as is. |
|
Note that by making GridSearchCV(..., param_grid=[{
'steps': [[('a', LinearSVC())]],
'a__C': [1, 10, 100],
},
{
'steps': [[('a', DecisionTreeClassifier())]],
'a__max_depth': [1, 5, 10],
}]I don't think the solution here ensures the correct ordering of setting |
samzhang111
force-pushed
the
param_invariance
branch
from
4e3e8a0
to
d160939
Compare
|
@jnothman Any motion toward merging it? I think that's a superset over this patch. The tests should still hold though. |
- Previously params in a FeatureUnion or Pipeline were different when deep=False and deep=True, making it impossible to set shallow params during GridSearch (issue scikit-learn#4461) - Added test to enforce deep=True results to be superset of deep=False. - Updated deep get_param output in pipeline estimators with shallow params. TST: Test estimators for get_params invariance - Estimators that support get_params should always be consistent in that get_params(deep=False) should be a subset of get_params(deep=True) - Implemented test over all "normal" and "other" estimators provided by all_estimators - Ignoring grid_search estimators for now TST: Amended init_pipeline test to only compare non-estimators
samzhang111
force-pushed
the
param_invariance
branch
from
d160939
to
edf60c0
Compare
|
|
|
This looks good to me. |
amueller
changed the title
| @@ -377,3 +378,15 @@ def test_transformer_n_iter(): | |||
|
|
|||
| if hasattr(estimator, "max_iter") and name not in external_solver: | |||
| yield check_transformer_n_iter, name, estimator | |||
There was a problem hiding this comment.
pep8
2 lines between functions
|
#4450 only has your +1, so we could merge this one first. |
[MRG + 1] BUG: get_param in FeatureUnion disjoint for shallow/deep params
|
merging, thanks :) |
when deep=False and deep=True, making it impossible to set shallow
params during GridSearch (issue Searching across FeatureUnion transformer_weights in a GridSearchCV #4461)
TST: Test estimators for get_params invariance (issue #4465)
in that get_params(deep=False) should be a subset of get_params(deep=True)
all_estimators