Paging #426
Paging #426
Conversation
…fset key from the filter dict
mfeurer
left a comment
mfeurer
left a comment
There was a problem hiding this comment.
Could you please add documentation for the function arguments which explains why they can be set separately (i.e. not as part of the kwargs)?
openml/datasets/functions.py
Outdated
|
|
||
| if size is not None: | ||
| api_call += "/limit/%d" % int(size) | ||
| return openml.utils.list_all(_list_datasets, offset=offset, size=size, status=status, **kwargs) |
There was a problem hiding this comment.
This looks to be >80 lines. Could you please try to stick to PEP8 as much as possible?
There was a problem hiding this comment.
True, I will keep the line length under 80. Regarding the arguments documentation, I kept the arguments as they were since we did not discuss any changes to them. In this case they can be included in the **kwargs as they are not positional and can all be set in one go. However, in other functions when we have lists, I would say we keep the list arguments separate from the primitives.
There was a problem hiding this comment.
Hm, given the structure of the REST API and the fact that the usage of the method right now is really unclear, it would be good if revert function signature. Also, I think calling it **filters instead of **kwargs will make the whole thing more similar to the REST API.
openml/datasets/functions.py
Outdated
| xml_string = _perform_api_call(api_call) | ||
| except OpenMLServerNoResult: | ||
| return dict() | ||
| except OpenMLServerNoResult as e: |
There was a problem hiding this comment.
Is there a reason why you changed this? I think returning a dict is more natural because it behaves like when listing an empty directory.
There was a problem hiding this comment.
Also, it appears to me that this piece of code is repeated in all listing functions. Could you please put that code into the utils module?
There was a problem hiding this comment.
There is a reason. It breaks the way that the list method in the utils works. As an example, without a limit, the previous implementation would never throw an error and we would be stuck in a loop.
In my opinion we should not change that, as we are performing checks and also actually creating the dict there. We can however remove the try and except as we are actually catching in the utils method.
There was a problem hiding this comment.
Basically utils.list_all -> loops list* -> forms the api call and passes it to the _list* function.
With the current flow it is not possible to do that.
There was a problem hiding this comment.
Okay, got it, that makes sense. Thanks for removing the try/except, this should make the code a lot easier.
Codecov Report
@@ Coverage Diff @@
## develop #426 +/- ##
===========================================
+ Coverage 88.96% 89.53% +0.57%
===========================================
Files 32 32
Lines 2682 2676 -6
===========================================
+ Hits 2386 2396 +10
+ Misses 296 280 -16
Continue to review full report at Codecov.
|
…ask_type for runs, filter by multipple operator for tasks and also refactored the code according to PEP8
Incorporated paging in the list_* functions #358. Refactored the code.