comments required by reviewer of PR

Author: janvanrijn

openml/flows/sklearn_converter.py

@@ -43,13 +43,6 @@ def sklearn_to_flow(o, parent_model=None):
         # TODO: explain what type of parameter is here
         rval = [sklearn_to_flow(element, parent_model) for element in o]
         if isinstance(o, tuple):
-            assert(parent_model is not None)
-            reserved_keywords = set(parent_model.get_params(deep=False).keys())
-            if o[0] in reserved_keywords:
-                parent_model_name = parent_model.__module__ + "." + \
-                                    parent_model.__class__.__name__
-                raise PyOpenMLError('Found element shadowing official ' + \
-                                    'parameter for %s: %s' %(parent_model_name, o[0]))
             rval = tuple(rval)
     elif isinstance(o, (bool, int, float, six.string_types)) or o is None:
         # base parameter values
@@ -278,12 +271,20 @@ def _extract_information_from_model(model):
                 isinstance(rval[0], (list, tuple)) and
                 [type(rval[0]) == type(rval[i]) for i in range(len(rval))]):
 
-            # Steps in a pipeline or feature union
+            # Steps in a pipeline or feature union, or base classifiers in voting classifier
             parameter_value = list()
+            reserved_keywords = set(model.get_params(deep=False).keys())
+
             for sub_component_tuple in rval:
                 identifier, sub_component = sub_component_tuple
                 sub_component_type = type(sub_component_tuple)
 
+                if identifier in reserved_keywords:
+                    parent_model_name = model.__module__ + "." + \
+                                        model.__class__.__name__
+                    raise PyOpenMLError('Found element shadowing official ' + \
+                                        'parameter for %s: %s' % (parent_model_name, identifier))
+
                 if sub_component is None:
                     # In a FeatureUnion it is legal to have a None step
 

tests/test_flows/test_sklearn.py

@@ -530,18 +530,28 @@ def test_illegal_parameter_names(self):
         clf1 = sklearn.ensemble.VotingClassifier(
             estimators=[('estimators', sklearn.ensemble.RandomForestClassifier()),
                         ('whatevs', sklearn.ensemble.ExtraTreesClassifier())])
-
-
-        cases = [clf1]
+        clf2 = sklearn.ensemble.VotingClassifier(
+            estimators=[('whatevs', sklearn.ensemble.RandomForestClassifier()),
+                        ('estimators', sklearn.ensemble.ExtraTreesClassifier())])
+        cases = [clf1, clf2]
 
         for case in cases:
             self.assertRaises(PyOpenMLError, sklearn_to_flow, case)
 
-    def test_illegal_parameter_names_sklearn(self):
+    def test_illegal_parameter_names_pipeline(self):
         # illegal name: steps
         steps = [
             ('Imputer', sklearn.preprocessing.Imputer(strategy='median')),
             ('OneHotEncoder', sklearn.preprocessing.OneHotEncoder(sparse=False, handle_unknown='ignore')),
             ('steps', sklearn.ensemble.BaggingClassifier(base_estimator=sklearn.tree.DecisionTreeClassifier))
         ]
-        self.assertRaises(ValueError, sklearn.pipeline.Pipeline, {'steps': steps })
+        self.assertRaises(ValueError, sklearn.pipeline.Pipeline, steps=steps)
+
+
+    def test_illegal_parameter_names_featureunion(self):
+        # illegal name: transformer_list
+        transformer_list = [
+            ('transformer_list', sklearn.preprocessing.Imputer(strategy='median')),
+            ('OneHotEncoder', sklearn.preprocessing.OneHotEncoder(sparse=False, handle_unknown='ignore'))
+        ]
+        self.assertRaises(ValueError, sklearn.pipeline.FeatureUnion, transformer_list=transformer_list)