-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
FIX: save epochs.event_id dict to fiff and read back #243
Conversation
- formally it works - but the dict serialized is not resotred on readin back - any thoughts? Example test: ``` Python run examples/plot_read_epochs.py epochs.event_id.update({'a': 1}) print epochs.event_id epochs.save('this_cool_feature.fif') e2.read('this_cool_feature.fif') print e2.event_id ```
@@ -70,6 +70,8 @@ def _read_events_fif(fid, tree): | |||
fid.close() | |||
raise ValueError('Could not find event data') | |||
|
|||
mappings = dir_tree_find(tree, FIFF.FIFF_DESCRIPTION) |
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.
you should look for it in the event block to avoid problems.
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.
Right, thanks!
can you add tests? |
Sure, I'm on it. What to do with read_events. As visible, I modified _read_events_fif to also get the mappings. For not breaking the api in terms of additional return values I blocked this behaviour for read_events where I untuple the return value of _read_events_fif but don't pass it. Two options I see: return mappings as well and deprecate / update examples or add anoother highlevel interface get_event_mappings or so. Makes sense? |
-- what about read events? see post above; besides this this is ready for review.
@@ -119,7 +135,7 @@ def read_events(filename, include=None, exclude=None): | |||
ext = splitext(filename)[1].lower() | |||
if ext == '.fif' or ext == '.gz': | |||
fid, tree, _ = fiff_open(filename) | |||
event_list = _read_events_fif(fid, tree) | |||
event_list, _ = _read_events_fif(fid, tree) |
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.
This is an intermediate solution. We could make this pass the mappings and deprecate or add another high-level mapping_getter_function. Wdyt?
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.
Actually I'm thinking this is fine for now. The only use case would be saving and reading events with mappings but without epochs.
LGTM +1 for merge. |
FIX: save epochs.event_id dict to fiff and read back - thanks for raising the issue!
Example test: