-
Notifications
You must be signed in to change notification settings - Fork 175
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
Improve splitting of datasets #147
Conversation
would you ming add some test @gemeinl ? can this be illustrated in some documentation / example? |
Not at all. I added an example as well as some tests. |
braindecode/datasets/base.py
Outdated
@@ -109,34 +109,37 @@ def __init__(self, list_of_ds): | |||
super().__init__(list_of_ds) | |||
self.description = pd.DataFrame([ds.description for ds in list_of_ds]) | |||
|
|||
def split(self, some_property=None, split_ids=None): | |||
"""Split the dataset based on some property listed in its description | |||
def split(self, by): |
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.
technically you are breaking public API here again. Maybe we want to reach a proper version
before stabilizing API. I feel there are still many moving parts.
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.
My bad, I am not yet used to deprecating and not breaking the API. And yes, a lot of stuff is moving.
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.
@gemeinl if you care about the design of the API it's great. Don't worry for now about breaking API as long as it's for the best.
# | ||
# License: BSD (3-clause) | ||
|
||
from IPython.display import display |
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.
why do you need this?
adding a dependency on ipython to do a print seems an overkill to me.
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.
I just copied it from examples.plot_dataset_example.py, so the dependency was already there. Could of course remove it everywhere.
I just copied it from examples.plot_dataset_example.py, so the dependency was already there. Could of course remove it everywhere.
I would do this. Maybe in another PR if you want
|
0a0f984
to
67d09a5
Compare
…of integers, or a list of list of integers. will now always return a dictionary with string keys
…ced new argument 'by'.
-fixed splitting tests
67d09a5
to
8a107fb
Compare
split()
now has a single argumentby
which ca be a string, a list of integers, or a list of list of integers.splitting now always returns a dictionary with string keys.