Add #737 #747
Add #737 #747
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #747 +/- ##
===========================================
- Coverage 87.97% 87.91% -0.06%
===========================================
Files 36 36
Lines 4090 4114 +24
===========================================
+ Hits 3598 3617 +19
- Misses 492 497 +5
Continue to review full report at Codecov.
|
# Conflicts: # doc/progress.rst # tests/test_flows/test_flow_functions.py
| @unittest.skipIf(LooseVersion(sklearn.__version__) == "0.19.1", | ||
| reason="Target flow is from sklearn 0.19.1") | ||
| def test_get_flow_reinstantiate_model_wrong_version(self): | ||
| openml.config.server = self.production_server |
There was a problem hiding this comment.
This is code was re-introduced in your merge, please remove it.
| openml.config.server = self.production_server | ||
| flow_id = 405 | ||
| size = 100 | ||
| evals_setups = openml.evaluations.list_evaluations_setups("predictive_accuracy", |
There was a problem hiding this comment.
I know we're generally not very good at code re-use, particularly in unit tests, but I'd rather change that :) Please extract the common code between test_list_evaluations_setups_filter_flow and test_list_evaluations_setups_filter_task into one function that is called from both tests (e.g. specifying either flow=[flow_id] or task=[task_id] through **kwargs).
| # Check if list is non-empty | ||
| self.assertGreater(len(evals_setups), 0) | ||
| # Check if output from sort is sorted in the right order | ||
| self.assertTrue(sorted(list(evals_setups['value'].values), reverse=True) |
There was a problem hiding this comment.
Please use assertEqual for equality checks. Edit: actually, as you're checking sequence equality, use assertSequenceEquals.
| self.assertTrue(sorted(list(evals_setups['value'].values), reverse=True) | ||
| == list(evals_setups['value'].values)) | ||
| # Check if output and order of list_evaluations is preserved | ||
| self.assertTrue((evals_setups['run_id'].values == evals['run_id'].values).all()) |
There was a problem hiding this comment.
Please use assertSequenceEquals, but you will also need to first use tolist
PGijsbers
left a comment
PGijsbers
left a comment
There was a problem hiding this comment.
Looks good, some minor code quality changes suggested. I think we could look into adding the hyperparameters as columns if a flow filter is applied, but perhaps that's something for a different PR?
A new column to the list_evaluations call? I am not sure if that is needed now that we have this new call, and I believe it may be better to do it in a different PR. |
|
To the returned dataframe of |
What does this PR implement/fix? Explain your changes.
add a list_evaluations_setups call that merges list_evaluations with list_setups and returns a list of evaluations with hyperparameters.
How should this PR be tested?
evaluations.list_evaluations_setups(function=function,flow=[flow_id], sort_order=sort_order,size=size, output_format='dataframe')
Any other comments?
list_evaluations has been modified. This is because the existing conversion to dataframe format does not preserve row order (even if ordered dict is passed - bug in pandas). So dataframe conversion is now performed by using a loop over all rows and collecting them in a list.