Repository navigation
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #523 +/- ##
===========================================
- Coverage 90.65% 89.88% -0.77%
===========================================
Files 32 32
Lines 2996 2858 -138
===========================================
- Hits 2716 2569 -147
- Misses 280 289 +9
Continue to review full report at Codecov.
|
janvanrijn
changed the title
|
@mfeurer Finally I got the PEP8 stuff fixed, can we merge before it breaks again? ;) |
| ('dummy', DummyClassifier(strategy='prior'))]) | ||
| self._run_and_upload(pipeline1, '62501') | ||
|
|
||
| @pytest.mark.skipif(LooseVersion(sklearn.__version__) < "0.20", |
There was a problem hiding this comment.
Is there a reason to use a method from pytest here instead of the respective one from unittest?
There was a problem hiding this comment.
It was my understanding that unittest is no longer supported and that we move to pytest? Correct me if I am wrong
There was a problem hiding this comment.
No, it's actually nosetests which got deprecated, unittest is a standard module (with somewhat limited functionality, that's we used nose and now use pytest).
There was a problem hiding this comment.
Cool, I replaced them.
| if not isinstance(sub_component, (OpenMLFlow, type(None))): | ||
| msg = 'Second item of tuple does not match assumptions. '\ | ||
| 'Expected OpenMLFlow, got %s' % type(sub_component) | ||
| raise ValueError(msg) |
There was a problem hiding this comment.
Could you please make this a TypeError?
| cr_value['key'] = identifier | ||
| cr_value['step_name'] = identifier | ||
| if len(sub_component_tuple) == 3: | ||
| cr_value['param_1'] = sub_component_tuple[2] |
There was a problem hiding this comment.
Should we maybe rename param_1 into argument_1 to as this is (in my opinion) rather an argument to that specific step of the pipeline than a parameter (or hyperparameter).
| if not isinstance(_tmp, (list, tuple)): | ||
| raise e | ||
| for step_name, step in _tmp: | ||
| for _temp_step in _tmp: |
There was a problem hiding this comment.
Maybe we should also add some checks/safeguards that the third argument is not an instance of OpenMLFlow? Meybe this piece of code can be generalized to check all elements in that list?
There was a problem hiding this comment.
I added checks for all 3 parts of the tuple
| self.assertEqual(new_model_params, fu_params) | ||
| new_model.fit(self.X, self.y) | ||
|
|
||
| @pytest.mark.skipif(LooseVersion(sklearn.__version__) < "0.20", |
There was a problem hiding this comment.
Please see comment below.
| serialization2 = sklearn_to_flow(new_model) | ||
| assert_flows_equal(serialization, serialization2) | ||
|
|
||
| @pytest.mark.skipif(LooseVersion(sklearn.__version__) < "0.20", |
|
Awesome! |
|
This is really great, from my POV we are ready to release now (although merging #482 would be great) |
|
sweet :) |
4 participants
Reference Issue
What does this PR implement/fix? Explain your changes.
How should this PR be tested?
Any other comments?