-
-
Notifications
You must be signed in to change notification settings - Fork 211
Removing split pickling #470
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
mfeurer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to not remove all lines you touched?
openml/tasks/split.py
Outdated
| np.array(repetitions[repetition][fold][sample][0], dtype=np.int32), | ||
| np.array(repetitions[repetition][fold][sample][1], dtype=np.int32)) | ||
|
|
||
| ''' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PyCharm usually wants double quotes, right?
|
@mfeurer since you mentioned removing or fixing split pickling, I only commented out the code as a simple solution. I can remove it completely if you are ok with it. @janvanrijn we are using double quotes for docstrings. |
|
I think removing it is fine. Reading those files isn't as costly as reading in the data. |
mfeurer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please remove the if cache statements and the cache=True. I think they're not necessary and complicate the code quite a bit.
|
@mfeurer sure, but we will not have a way to disable loading from cache. |
|
I don't think we have this for anything else. As a split should never change I don't see a reason to keep this functionality. |
|
@mfeurer while working in the java repo, there was a case when the server was having problems and it was returning a bad value for a certain request. The value was cached and it would fail a procedure we were running as it would run on the cached object. In this case the flag was necessary (apart from the fact that the files could be removed locally). However given that we validate the object, this should not happen. |
|
Yes, we have this piece of code for downloading the task: One last thing, could you please remove the try/except around the pickle file loading. We don't react on any exception, so the try/except is actually useless. |
Issue #431