New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRG+1] fix _BaseComposition._set_params with nested parameters #9945

Merged
merged 12 commits into from Oct 18, 2017
Next

iterate over parameters in sorted order in ``set_params``

  • Loading branch information...
amueller committed Oct 17, 2017
commit 16526cc7aef149ffb606492ef80a388ff97c8405
View
@@ -248,7 +248,10 @@ def set_params(self, **params):
# Simple optimization to gain speed (inspect is slow)
return self
valid_params = self.get_params(deep=True)
for key, value in six.iteritems(params):
# ensure ordered iteration
keys = sorted(params)
for key in keys:
value = params[key]
split = key.split('__', 1)
if len(split) > 1:
# nested objects case
@@ -24,10 +24,11 @@
from sklearn.base import clone, BaseEstimator
from sklearn.pipeline import Pipeline, FeatureUnion, make_pipeline, make_union
from sklearn.svm import SVC
from sklearn.linear_model import LogisticRegression
from sklearn.linear_model import LogisticRegression, Lasso
from sklearn.linear_model import LinearRegression
from sklearn.cluster import KMeans
from sklearn.feature_selection import SelectKBest, f_classif
from sklearn.dummy import DummyRegressor
from sklearn.decomposition import PCA, TruncatedSVD
from sklearn.datasets import load_iris
from sklearn.preprocessing import StandardScaler
@@ -863,6 +864,20 @@ def test_step_name_validation():
[[1]], [1])
def test_set_conditional_params():
estimator = Pipeline([
('a', Pipeline([
('b', DummyRegressor())
]))
])
params = {

This comment has been minimized.

@lesteve

lesteve Oct 18, 2017

Member

Should we use a collections.OrderedDict to make this test more future-proof?

@lesteve

lesteve Oct 18, 2017

Member

Should we use a collections.OrderedDict to make this test more future-proof?

This comment has been minimized.

@jnothman

jnothman Oct 18, 2017

Member

Shrug. I think it distorts the purpose of the test somewhat if we only test with an OrderedDict.

@jnothman

jnothman Oct 18, 2017

Member

Shrug. I think it distorts the purpose of the test somewhat if we only test with an OrderedDict.

This comment has been minimized.

@lesteve

lesteve Oct 18, 2017

Member

I think this is a bit clearer (without some of the subtlety of python version-specific dict ordering):

estimator.set_params(a__b__alpha=0.001, a__b=Lasso())
@lesteve

lesteve Oct 18, 2017

Member

I think this is a bit clearer (without some of the subtlety of python version-specific dict ordering):

estimator.set_params(a__b__alpha=0.001, a__b=Lasso())
'a__b__alpha': 0.001,
'a__b': Lasso(),
}
estimator.set_params(**params)
def test_pipeline_wrong_memory():
# Test that an error is raised when memory is not a string or a Memory
# instance
ProTip! Use n and p to navigate between commits in a pull request.