WIP: More extensive unit tests for run to / from xml by mfeurer · Pull Request #482 · openml/openml-python

mfeurer · 2018-06-19T12:14:54Z

This is a first step towards more extensive unit tests for to/from xml for runs, checking that the uploaded xml file and the xml file generated from a downloaded run are equal.

Requires more extensive rewrites of how traces are generated to check them as well.

Needs to be rebased on #481 to not choke on wrong sorting in the xml file.

…unctions as trace methods.

mfeurer

Looks good, but please have a look at my comments.

mfeurer · 2018-07-03T11:12:52Z

+                    dataset_id=dataset.dataset_id, model=flow.model, tags=tags,
+                    trace_content=trace_content, trace_attributes=trace_attributes,
+                    data_content=data_content)
    run.parameter_settings = OpenMLRun._parse_parameters(flow)


Could you, for the future, please format like:

OpenMLRun( arg1=arg1, arg2=arg2, )

This will make reviewing a lot easier.

Sure, this time I just followed the 'style' from the previous implementation.

mfeurer · 2018-07-03T11:14:45Z

+        try:
+            self.trace = self._generate_trace(trace_attributes, trace_content)
+        except ValueError:
+            self.trace = None


Could you please make the failure more transparent to the user? For example if the trace_attributes and trace_content are empty set self.trace to None as done right now and actually raise the exception otherwise?

mfeurer · 2018-07-03T11:15:31Z

-        """Generates the arff dictionary for uploading predictions to the server.
-
-        Assumes that the run has been executed.
+    def _generate_trace(self, trace_attributes, trace_content):


Why did you remove the docstring?

Because the function changed, it does not return an arff dict but a trace instead. I can however add a docstring for the new function.

mfeurer · 2018-07-03T11:17:37Z

+        if trace_content is None or len(trace_content) == 0:
            raise ValueError('No trace content available.')
-        if len(self.trace_attributes) != len(self.trace_content[0]):
+        if trace_attributes is None or len(trace_attributes) != len(trace_content[0]):


I assume that trace_attributes is None will happen quite often. Do you maybe mean trace_attributes is not None?

I meant is None. I wanted to partially controll the error, otherwise when trace_attributes would be None the len function would throw by itself. I can however handle this better

mfeurer · 2018-07-03T11:20:03Z

+        if self.trace_iterations is None:
+            raise ValueError("trace_iterations missing from the trace object")
+        # attributes that will be in trace arff
+        trace_attributes = [('repeat', 'NUMERIC'),


Could you please reformat this as mentioned above?

mfeurer · 2018-07-03T11:21:02Z

+            evaluation = float(itt[attribute_idx['evaluation']])
+            setup_string = None
+            if flag_ss:
+                if itt[attribute_idx['setup_string']] != None:


Why not is not None?

you are right, it should be 'is not None.'

mfeurer · 2018-07-03T11:25:13Z

+
+    @classmethod
+    def _trace_from_xml(cls, xml):
+        """Generate trace from xml.


Can you generalize this and the method above?

Generalizing is not easy, since they use different attribute names and also one is a list while the other one is a dictionary. Skipping it for the moment.

Okay. Could you please add a comment regarding that so that a future maintainer already knows about this.

ArlindKadra · 2018-07-20T07:03:30Z

Sure, I am waiting for the response on the issue that I created at OpenML repo and then I can add the comment along with any possible changes.

…

On Thu, Jul 19, 2018, 15:26 Matthias Feurer ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In openml/runs/trace.py <#482 (comment)>: > + + # if someone needs it, he can use the parameter + # fields to revive the setup_string as well + # However, this is usually done by the OpenML server + # and if we are going to duplicate this functionality + # it needs proper testing + + current = OpenMLTraceIteration(repeat, fold, iteration, setup_string, + evaluation, selected) + trace[(repeat, fold, iteration)] = current + + return cls(None, trace) + + @classmethod + def _trace_from_xml(cls, xml): + """Generate trace from xml. Okay. Could you please add a comment regarding that so that a future maintainer already knows about this. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#482 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AXiTK0qb2r6-pewv6-fEoJ4hvvECg8-kks5uIHrqgaJpZM4UtY7g> .

janvanrijn · 2018-08-15T16:57:42Z

The reason why the PR fails is the following:

Look at the trace file that I generated using the development branch: https://test.openml.org

@RELATION openml_task_119_predictions

@ATTRIBUTE repeat NUMERIC
@ATTRIBUTE fold NUMERIC
@ATTRIBUTE iteration NUMERIC
@ATTRIBUTE evaluation NUMERIC
@ATTRIBUTE selected {true, false}
@ATTRIBUTE parameter_min_samples_split STRING
@ATTRIBUTE parameter_max_depth STRING
@ATTRIBUTE parameter_bootstrap STRING
@ATTRIBUTE parameter_max_features STRING
@ATTRIBUTE parameter_min_samples_leaf STRING
@ATTRIBUTE parameter_criterion STRING

@DATA
0,0,0,0.7786407766990291,true,6,3,true,3,2,'\"gini\"'
0,0,1,0.6990291262135923,false,6,null,true,2,3,'\"gini\"'
0,0,2,0.7184466019417476,false,3,null,false,4,9,'\"entropy\"'
0,0,3,0.7242718446601941,false,7,null,true,3,1,'\"entropy\"'
0,0,4,0.7378640776699029,false,9,3,false,3,6,'\"entropy\"'

And the trace file with this specific branch:

@RELATION Trace

@ATTRIBUTE repeat NUMERIC
@ATTRIBUTE fold NUMERIC
@ATTRIBUTE iteration NUMERIC
@ATTRIBUTE evaluation NUMERIC
@ATTRIBUTE selected {true, false}
@ATTRIBUTE setup_string STRING

@DATA
0,0,0,0.658252427184,false,?
0,0,1,0.658252427184,false,?
0,0,2,0.658252427184,false,?
0,0,3,0.658252427184,false,?
0,0,4,0.658252427184,false,?
0,0,5,0.658252427184,false,?
0,0,6,0.660194174757,true,?
0,0,7,0.658252427184,false,?
0,0,8,0.658252427184,false,?

I see the following differences. The original one has several parameters registered with prefix parameter_ . The new one tries to do the same with setup_string, but this is not the same.

I suggest to remove the setup_string, and return to the individual parameter_ fields. I will improve error messages thrown by the evaluation engine (due to a bug, it only showed "java error". This will be better after the latest update is merged).

This reverts commit a150e27.

mfeurer · 2018-09-13T07:33:26Z

@janvanrijn could you please use a different branch for these changes? This branch for improving the trace handling for the current version of scikit-learn, which already appears to be quite difficult.

mfeurer · 2018-09-13T09:21:58Z

Okay, I fixed the previous error and will now work on removing the new error. Afterwards, this will need a bit of cleanup.

janvanrijn · 2018-09-13T13:36:09Z

excuse me, wrong branch. i will move them

janvanrijn · 2018-09-26T15:52:34Z

I will start my review now (in train, wifi seems unstable). Please note additionally that the unit tests fail, all on test cases that seem relevant:

=================================== FAILURES ===================================
____________________ TestTrace.test_get_selected_iteration _____________________
[gw0] linux2 -- Python 2.7.15 /home/travis/miniconda/envs/testenv/bin/python
self = <tests.test_runs.test_trace.TestTrace testMethod=test_get_selected_iteration>
    def test_get_selected_iteration(self):
        trace_iterations = {}
        for i in range(5):
            for j in range(5):
                for k in range(5):
                    t = OpenMLTraceIteration(
                        repeat=i,
                        fold=j,
                        iteration=5,
                        setup_string='parameter_%d%d%d' % (i, j, k),
                        evaluation=1.0 * i + 0.1 * j + 0.01 * k,
                        selected=(i == j and i == k and i == 2),
                        paramaters=None,
                    )
                    trace_iterations[(i, j, k)] = t
    
        trace = OpenMLRunTrace(-1, trace_iterations=trace_iterations)
        # This next one should simply not fail
        self.assertEqual(trace.get_selected_iteration(2, 2), 2)
>       with self.assertRaisesRegex(
            ValueError,
                'Could not find the selected iteration for rep/fold 3/3',
        ):
E       AttributeError: 'TestTrace' object has no attribute 'assertRaisesRegex'
/home/travis/build/openml/openml-python/tests/test_runs/test_trace.py:26: AttributeError
________________________ TestTrace.test_initialization _________________________
[gw0] linux2 -- Python 2.7.15 /home/travis/miniconda/envs/testenv/bin/python
self = <tests.test_runs.test_trace.TestTrace testMethod=test_initialization>
    def test_initialization(self):
        """Check all different ways to fail the initialization """
        rval = OpenMLRunTrace.generate(None, None)
        self.assertIsNone(rval)
>       with self.assertRaisesRegex(
            ValueError,
            'Trace content not available.',
        ):
E       AttributeError: 'TestTrace' object has no attribute 'assertRaisesRegex'
/home/travis/build/openml/openml-python/tests/test_runs/test_trace.py:37: AttributeError
___________________ TestRun.test_run_and_upload_randomsearch ___________________
[gw1] linux2 -- Python 2.7.15 /home/travis/miniconda/envs/testenv/bin/python
self = <tests.test_runs.test_run_functions.TestRun testMethod=test_run_and_upload_randomsearch>
    def test_run_and_upload_randomsearch(self):
        randomsearch = RandomizedSearchCV(
            RandomForestClassifier(n_estimators=5),
            {"max_depth": [3, None],
             "max_features": [1, 2, 3, 4],
             "min_samples_split": [2, 3, 4, 5, 6, 7, 8, 9, 10],
             "min_samples_leaf": [1, 2, 3, 4, 5, 6, 7, 8, 9, 10],
             "bootstrap": [True, False],
             "criterion": ["gini", "entropy"]},
            cv=StratifiedKFold(n_splits=2, shuffle=True),
            n_iter=5)
        # The random states for the RandomizedSearchCV is set after the
        # random state of the RandomForestClassifier is set, therefore,
        # it has a different value than the other examples before
>       self._run_and_upload(randomsearch, '12172')
/home/travis/build/openml/openml-python/tests/test_runs/test_run_functions.py:405: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/home/travis/build/openml/openml-python/tests/test_runs/test_run_functions.py:358: in _run_and_upload
    check_res = self._check_serialized_optimized_run(run.run_id)
/home/travis/build/openml/openml-python/tests/test_runs/test_run_functions.py:89: in _check_serialized_optimized_run
    seed=1)
/home/travis/build/openml/openml-python/openml/runs/functions.py:48: in run_model_on_task
    add_local_measures=add_local_measures)
/home/travis/build/openml/openml-python/openml/runs/functions.py:119: in run_flow_on_task
    res = _run_task_get_arffcontent(flow.model, task, add_local_measures=add_local_measures)
/home/travis/build/openml/openml-python/openml/runs/functions.py:458: in _run_task_get_arffcontent
    add_local_measures=add_local_measures)
/home/travis/build/openml/openml-python/openml/runs/functions.py:572: in _run_model_on_fold
    model.fit(trainX, trainY)
/home/travis/miniconda/envs/testenv/lib/python2.7/site-packages/sklearn/ensemble/forest.py:326: in fit
    for i, t in enumerate(trees))
/home/travis/miniconda/envs/testenv/lib/python2.7/site-packages/sklearn/externals/joblib/parallel.py:758: in __call__
    while self.dispatch_one_batch(iterator):
/home/travis/miniconda/envs/testenv/lib/python2.7/site-packages/sklearn/externals/joblib/parallel.py:608: in dispatch_one_batch
    self._dispatch(tasks)
/home/travis/miniconda/envs/testenv/lib/python2.7/site-packages/sklearn/externals/joblib/parallel.py:571: in _dispatch
    job = self._backend.apply_async(batch, callback=cb)
/home/travis/miniconda/envs/testenv/lib/python2.7/site-packages/sklearn/externals/joblib/_parallel_backends.py:109: in apply_async
    result = ImmediateResult(func)
/home/travis/miniconda/envs/testenv/lib/python2.7/site-packages/sklearn/externals/joblib/_parallel_backends.py:326: in __init__
    self.results = batch()
/home/travis/miniconda/envs/testenv/lib/python2.7/site-packages/sklearn/externals/joblib/parallel.py:131: in __call__
    return [func(*args, **kwargs) for func, args, kwargs in self.items]
/home/travis/miniconda/envs/testenv/lib/python2.7/site-packages/sklearn/ensemble/forest.py:120: in _parallel_build_trees
    tree.fit(X, y, sample_weight=curr_sample_weight, check_input=False)
/home/travis/miniconda/envs/testenv/lib/python2.7/site-packages/sklearn/tree/tree.py:739: in fit
    X_idx_sorted=X_idx_sorted)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
self = DecisionTreeClassifier(class_weight=None, criterion=10, max_depth=6,
         ...n_leaf=0.0, presort=False,
            random_state=451974009, splitter='best')
X = array([[3.00e+00, 1.74e+02, 5.80e+01, ..., 3.29e+01, 5.93e-01, 3.60e+01],
    ..., 1.03e+02, 6.60e+01, ..., 3.91e+01, 3.44e-01, 3.10e+01]],
      dtype=float32)
y = array([[1.],
       [0.],
       [0.],
       [0.],
       [1.],
       [0.],
...
       [1.],
       [1.],
       [1.],
       [0.],
       [0.],
       [1.]])
sample_weight = array([0., 1., 3., 1., 2., 2., 1., 0., 0., 0., 0., 1., 1., 1., 1., 1., 2.,
   ...0., 0., 3., 0., 1., 1., 0., 1., 1., 2., 1., 3., 3.,
       4., 1., 0., 0., 0.])
check_input = False, X_idx_sorted = None
    def fit(self, X, y, sample_weight=None, check_input=True,
            X_idx_sorted=None):
    
        random_state = check_random_state(self.random_state)
        if check_input:
            X = check_array(X, dtype=DTYPE, accept_sparse="csc")
            y = check_array(y, ensure_2d=False, dtype=None)
            if issparse(X):
                X.sort_indices()
    
                if X.indices.dtype != np.intc or X.indptr.dtype != np.intc:
                    raise ValueError("No support for np.int64 index based "
                                     "sparse matrices")
    
        # Determine output settings
        n_samples, self.n_features_ = X.shape
        is_classification = isinstance(self, ClassifierMixin)
    
        y = np.atleast_1d(y)
        expanded_class_weight = None
    
        if y.ndim == 1:
            # reshape is necessary to preserve the data contiguity against vs
            # [:, np.newaxis] that does not.
            y = np.reshape(y, (-1, 1))
    
        self.n_outputs_ = y.shape[1]
    
        if is_classification:
            check_classification_targets(y)
            y = np.copy(y)
    
            self.classes_ = []
            self.n_classes_ = []
    
            if self.class_weight is not None:
                y_original = np.copy(y)
    
            y_encoded = np.zeros(y.shape, dtype=np.int)
            for k in range(self.n_outputs_):
                classes_k, y_encoded[:, k] = np.unique(y[:, k],
                                                       return_inverse=True)
                self.classes_.append(classes_k)
                self.n_classes_.append(classes_k.shape[0])
            y = y_encoded
    
            if self.class_weight is not None:
                expanded_class_weight = compute_sample_weight(
                    self.class_weight, y_original)
    
        else:
            self.classes_ = [None] * self.n_outputs_
            self.n_classes_ = [1] * self.n_outputs_
    
        self.n_classes_ = np.array(self.n_classes_, dtype=np.intp)
    
        if getattr(y, "dtype", None) != DOUBLE or not y.flags.contiguous:
            y = np.ascontiguousarray(y, dtype=DOUBLE)
    
        # Check parameters
        max_depth = ((2 ** 31) - 1 if self.max_depth is None
                     else self.max_depth)
        max_leaf_nodes = (-1 if self.max_leaf_nodes is None
                          else self.max_leaf_nodes)
    
        if isinstance(self.min_samples_leaf, (numbers.Integral, np.integer)):
            if not 1 <= self.min_samples_leaf:
                raise ValueError("min_samples_leaf must be at least 1 "
                                 "or in (0, 0.5], got %s"
                                 % self.min_samples_leaf)
            min_samples_leaf = self.min_samples_leaf
        else:  # float
            if not 0. < self.min_samples_leaf <= 0.5:
                raise ValueError("min_samples_leaf must be at least 1 "
                                 "or in (0, 0.5], got %s"
                                 % self.min_samples_leaf)
            min_samples_leaf = int(ceil(self.min_samples_leaf * n_samples))
    
        if isinstance(self.min_samples_split, (numbers.Integral, np.integer)):
            if not 2 <= self.min_samples_split:
                raise ValueError("min_samples_split must be at least 2 "
                                 "or in (0, 1], got %s"
                                 % self.min_samples_split)
            min_samples_split = self.min_samples_split
        else:  # float
            if not 0. < self.min_samples_split <= 1.:
                raise ValueError("min_samples_split must be at least 2 "
                                 "or in (0, 1], got %s"
                                 % self.min_samples_split)
            min_samples_split = int(ceil(self.min_samples_split * n_samples))
            min_samples_split = max(2, min_samples_split)
    
        min_samples_split = max(min_samples_split, 2 * min_samples_leaf)
    
        if isinstance(self.max_features, six.string_types):
            if self.max_features == "auto":
                if is_classification:
                    max_features = max(1, int(np.sqrt(self.n_features_)))
                else:
                    max_features = self.n_features_
            elif self.max_features == "sqrt":
                max_features = max(1, int(np.sqrt(self.n_features_)))
            elif self.max_features == "log2":
                max_features = max(1, int(np.log2(self.n_features_)))
            else:
                raise ValueError(
                    'Invalid value for max_features. Allowed string '
                    'values are "auto", "sqrt" or "log2".')
        elif self.max_features is None:
            max_features = self.n_features_
        elif isinstance(self.max_features, (numbers.Integral, np.integer)):
            max_features = self.max_features
        else:  # float
            if self.max_features > 0.0:
                max_features = max(1,
                                   int(self.max_features * self.n_features_))
            else:
                max_features = 0
    
        self.max_features_ = max_features
    
        if len(y) != n_samples:
            raise ValueError("Number of labels=%d does not match "
                             "number of samples=%d" % (len(y), n_samples))
        if not 0 <= self.min_weight_fraction_leaf <= 0.5:
            raise ValueError("min_weight_fraction_leaf must in [0, 0.5]")
        if max_depth <= 0:
            raise ValueError("max_depth must be greater than zero. ")
        if not (0 < max_features <= self.n_features_):
            raise ValueError("max_features must be in (0, n_features]")
        if not isinstance(max_leaf_nodes, (numbers.Integral, np.integer)):
            raise ValueError("max_leaf_nodes must be integral number but was "
                             "%r" % max_leaf_nodes)
        if -1 < max_leaf_nodes < 2:
            raise ValueError(("max_leaf_nodes {0} must be either smaller than "
                              "0 or larger than 1").format(max_leaf_nodes))
    
        if sample_weight is not None:
            if (getattr(sample_weight, "dtype", None) != DOUBLE or
                    not sample_weight.flags.contiguous):
                sample_weight = np.ascontiguousarray(
                    sample_weight, dtype=DOUBLE)
            if len(sample_weight.shape) > 1:
                raise ValueError("Sample weights array has more "
                                 "than one dimension: %d" %
                                 len(sample_weight.shape))
            if len(sample_weight) != n_samples:
                raise ValueError("Number of weights=%d does not match "
                                 "number of samples=%d" %
                                 (len(sample_weight), n_samples))
    
        if expanded_class_weight is not None:
            if sample_weight is not None:
                sample_weight = sample_weight * expanded_class_weight
            else:
                sample_weight = expanded_class_weight
    
        # Set min_weight_leaf from min_weight_fraction_leaf
        if sample_weight is None:
            min_weight_leaf = (self.min_weight_fraction_leaf *
                               n_samples)
        else:
            min_weight_leaf = (self.min_weight_fraction_leaf *
                               np.sum(sample_weight))
    
        if self.min_impurity_split < 0.:
            raise ValueError("min_impurity_split must be greater than "
                             "or equal to 0")
    
        presort = self.presort
        # Allow presort to be 'auto', which means True if the dataset is dense,
        # otherwise it will be False.
        if self.presort == 'auto' and issparse(X):
            presort = False
        elif self.presort == 'auto':
            presort = True
    
        if presort is True and issparse(X):
            raise ValueError("Presorting is not supported for sparse "
                             "matrices.")
    
        # If multiple trees are built on the same dataset, we only want to
        # presort once. Splitters now can accept presorted indices if desired,
        # but do not handle any presorting themselves. Ensemble algorithms
        # which desire presorting must do presorting themselves and pass that
        # matrix into each tree.
        if X_idx_sorted is None and presort:
            X_idx_sorted = np.asfortranarray(np.argsort(X, axis=0),
                                             dtype=np.int32)
    
        if presort and X_idx_sorted.shape != X.shape:
            raise ValueError("The shape of X (X.shape = {}) doesn't match "
                             "the shape of X_idx_sorted (X_idx_sorted"
                             ".shape = {})".format(X.shape,
                                                   X_idx_sorted.shape))
    
        # Build tree
        criterion = self.criterion
        if not isinstance(criterion, Criterion):
            if is_classification:
>               criterion = CRITERIA_CLF[self.criterion](self.n_outputs_,
                                                         self.n_classes_)
E               KeyError: 10
/home/travis/miniconda/envs/testenv/lib/python2.7/site-packages/sklearn/tree/tree.py:317: KeyError
____________________ TestRun.test_run_and_upload_gridsearch ____________________
[gw3] linux2 -- Python 2.7.15 /home/travis/miniconda/envs/testenv/bin/python
self = <tests.test_runs.test_run_functions.TestRun testMethod=test_run_and_upload_gridsearch>
    def test_run_and_upload_gridsearch(self):
        gridsearch = GridSearchCV(BaggingClassifier(base_estimator=SVC()),
                                  {"base_estimator__C": [0.01, 0.1, 10],
                                   "base_estimator__gamma": [0.01, 0.1, 10]})
>       self._run_and_upload(gridsearch, '62501')
/home/travis/build/openml/openml-python/tests/test_runs/test_run_functions.py:389: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/home/travis/build/openml/openml-python/tests/test_runs/test_run_functions.py:358: in _run_and_upload
    check_res = self._check_serialized_optimized_run(run.run_id)
/home/travis/build/openml/openml-python/tests/test_runs/test_run_functions.py:102: in _check_serialized_optimized_run
    self.assertEqual(predictions['data'][idx][col_idx], predictions_prime['data'][idx][col_idx])
E   AssertionError: u'tested_positive' != u'tested_negative'
E   - tested_positive
E   ?        ^^^^
E   + tested_negative
E   ?        ^^^^
___________________ TestRun.test_run_and_upload_maskedarrays ___________________
[gw3] linux2 -- Python 2.7.15 /home/travis/miniconda/envs/testenv/bin/python
self = <tests.test_runs.test_run_functions.TestRun testMethod=test_run_and_upload_maskedarrays>
    def test_run_and_upload_maskedarrays(self):
        # This testcase is important for 2 reasons:
        # 1) it verifies the correct handling of masked arrays (not all parameters are active)
        # 2) it verifies the correct handling of a 2-layered grid search
        # Note that this is a list of dictionaries, all containing 1 hyperparameter.
        gridsearch = GridSearchCV(
            RandomForestClassifier(n_estimators=5),
            [
                {'max_features': [2, 4]},
                {'min_samples_leaf': [1, 10]}
            ],
            cv=StratifiedKFold(n_splits=2, shuffle=True)
        )
        # The random states for the GridSearchCV is set after the
        # random state of the RandomForestClassifier is set, therefore,
        # it has a different value than the other examples before
>       self._run_and_upload(gridsearch, '12172')
/home/travis/build/openml/openml-python/tests/test_runs/test_run_functions.py:423: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/home/travis/build/openml/openml-python/tests/test_runs/test_run_functions.py:358: in _run_and_upload
    check_res = self._check_serialized_optimized_run(run.run_id)
/home/travis/build/openml/openml-python/tests/test_runs/test_run_functions.py:102: in _check_serialized_optimized_run
    self.assertEqual(predictions['data'][idx][col_idx], predictions_prime['data'][idx][col_idx])
E   AssertionError: u'tested_negative' != u'tested_positive'
E   - tested_negative
E   ?        ^^^^
E   + tested_positive
E   ?        ^^^^
============== 5 failed, 193 passed, 4 skipped in 581.97 seconds ===============

janvanrijn · 2018-09-26T16:00:24Z

+        if len(content) == 0:
+            raise ValueError('Trace content is empty.')
+        if attributes is not None:
+            if len(attributes) != len(content[0]):


or: len(content.shape[1])

Content will be a list, so this will not work.

janvanrijn · 2018-09-26T16:01:36Z

+        trace = OrderedDict()
+        attribute_idx = {att[0]: idx for idx, att in enumerate(attributes)}
+
+        required_attributes = ['repeat', 'fold', 'iteration', 'evaluation',


maybe declare these in a more prominent place? (static class variable or global variable. Personally I prefer the static class variable)

Although this seems far fetched, we should also check the attributes array whether there are no attributes that carry any of these names (and add a unit test)

janvanrijn · 2018-09-26T16:03:24Z

    ----------
    run_id : int
-        OpenML run id
+        OpenML run id.


I don't care about the tailing dot, but why doesn't our PEP8 check make any noise? ;)

janvanrijn · 2018-09-26T16:04:43Z

+            ('selected', ['true', 'false']),
+        ]
+        trace_attributes.extend([
+            ('parameter_' + parameter, 'STRING') for parameter in


please use the constant (that might have become a static class variable by now) here

janvanrijn · 2018-09-26T16:07:15Z

+        ]

-    @staticmethod
-    def _from_filesystem(file_path):


Just asking, as I can't figure out from the code: Why did you remove this as a private function and add this to a massive generate function? I am (probably) fine with either

There's no reason, I made this private again.

janvanrijn · 2018-09-26T16:09:55Z

+        arff_dict = OrderedDict()
+        data = []
+        for trace_iteration in self.trace_iterations.values():
+            tit_list = []


suggestive variable name, maybe change to something less politically charged.

janvanrijn · 2018-09-26T16:10:17Z

+        for trace_iteration in self.trace_iterations.values():
+            tit_list = []
+            for attr, _ in trace_attributes:
+                if attr.startswith('parameter_'):


please use the constant

janvanrijn · 2018-09-26T16:14:23Z

+        attribute_idx = {
+            att[0]: idx for idx, att in enumerate(arff_obj['attributes'])
+        }
+        required_attributes = [


From here on, the code seems highly similar to the static function that you pulled in the generate function. Unless there is any good reason, why not put the duplicate code in a (static) functions?

janvanrijn · 2018-09-26T16:17:57Z

            att_name = trace_attribute_list[att_idx][0]
+            # They no longer start with parameter_ if they come from
+            # extract_arff_trace!
            if att_name.startswith("parameter_"):


please use the constant (which is a static class variable by now)

janvanrijn

I really like this PR! I know there are a lot of changes requested, but once these are resolved this highly uncomplicates the code.

mfeurer · 2018-09-27T09:27:23Z

@janvanrijn @ArlindKadra I updated this PR based on your suggestions. Please check again.

janvanrijn

Note that you left some TODO's in the unit tests as well.

mfeurer assigned ArlindKadra Jun 19, 2018

mfeurer mentioned this pull request Jun 20, 2018

Unit test and fix for #422 #481

Merged

ArlindKadra force-pushed the add/#465 branch from 86d2934 to fc008e4 Compare June 24, 2018 15:35

mfeurer and others added 11 commits June 24, 2018 17:31

run functions test: compare xml of uploaded and downloaded run

fc008e4

Initial changes

ce0694a

Refactored the run and trace classes. Refactored functions from run/f…

60dc43a

…unctions as trace methods.

First try at fixing unit tests

36f25ea

Fixing unit tests

949be6d

Fixing failing unit tests because of the run refactoring

fce97c3

Arff byte object expected fix

a25036a

Fixing OpenMLRun does not have attribute trace_content

76eb2c9

Fix NoneType has no attribute ...

31b8563

Fixing the addition of setup_string in trace

3e184f4

Fixing bug

43b6b62

mfeurer commented Jul 3, 2018

View reviewed changes

ArlindKadra added 2 commits July 18, 2018 19:25

Refactoring code

fc2ec97

Reverting changes to trace and fixing bugs

c2bf973

janvanrijn and others added 2 commits September 12, 2018 17:42

sklearn converter first attempt

a150e27

Revert "sklearn converter first attempt"

92cba02

This reverts commit a150e27.

mfeurer added 2 commits September 13, 2018 09:43

Merge branch 'develop' into add/#465

c21d16b

one step further to make this refactoring work

f5dfda5

MAINT fix non-refactored function calls

2993d10

mfeurer added 3 commits September 20, 2018 09:54

fix a few unit tests

630ef84

MAINT please the style checker

22d2f7b

Merge branch 'develop' into add/#465

0648d58

mfeurer added 4 commits September 21, 2018 12:30

improve code style

4903e86

work on arlind's suggestions

879dc3e

FIX type error and add missing file

dcfbc48

MAINT improve style

6f03812

mfeurer requested a review from janvanrijn September 21, 2018 18:54