Add MED wrapper #1288
Add MED wrapper #1288
Conversation
There was a problem hiding this comment.
Hi @dancrepeau!
Thanks for reworking the IO a second time. Now the structure and mapping to neo streams looks perfect.
I left a couple of small suggestions, comments and questions.
It also seems that you are only considering a part of record data to be loaded into events if these are of type Note or Nlx - is there potentially more data in those records that could be exposed via Neo?
Currently the tests are still failing due to a segmentation fault. Do you get the same error on your system and do you have any idea what's going wrong there?
| # found a match, so add it! | ||
| add_to_existing_stream_info = True | ||
| stream_info['chan_list'].append((chan_idx, chan_info['metadata']['channel_name'])) | ||
| stream_info['raw_chans'].append(chan_info['metadata']['channel_name']) |
There was a problem hiding this comment.
This is not a big performance improvement, but you could stop iterating here once you found the matching stream
| stream_info['raw_chans'].append(chan_info['metadata']['channel_name']) | |
| stream_info['raw_chans'].append(chan_info['metadata']['channel_name']) | |
| break |
There was a problem hiding this comment.
@dancrepeau Does this make sense to you or would you rather keep the code at it is?
There was a problem hiding this comment.
ok, let's put this also on the list of potential improvements for the future.
|
Hi @JuliaSprenger! As always, thanks for the feedback! I will look into the segfault in the next day or two and try to figure out what is going on. I do most of my testing on Mac (M1 chip) and everything is passing for me. Since this appears to be segfaulting on Ubuntu I will try to reproduce the problem there. |
|
Hi @dancrepeau Any news on the segfault? Do you have access to an ubuntu system to test this or is gh actions sufficient for you to tests this? |
|
Hi @JuliaSprenger! Yes, we tracked down the segfault and fixed it. The newest dhn_med_py version is 1.1.1 which has the fix in it. Thank you for being patient! I will go through the rest of your comments and update the MED wrapper in the near future. thanks! |
|
Hi @JuliaSprenger, We fixed the segfault in the dhn_med_py library, and I believe I have made the suggested adjustments to the code. Both RawIO and IO are passing the tests in my local testing. Any comments are welcome. Thanks! |
|
Hi @dancrepeau. |
|
@dancrepeau Is this ready to merge, or are there still comments you plan to address? |
|
Hi @apdavison, as far as I know, it is ready to be merged. Julia had a question about why MED is using the subset of events that we do (which I think I addressed) and suggested a speed-up for counting the number of events, although the only way I can think to perform that speedup in the parse_header routine would involve duplicating data that is already part of the MedSession python object. So it's your call - I'm happy with it, and if everyone else is, I'd say let's merge. Thanks! |
Hello,
This is the third attempt to add MED format (medformat.org) support into Neo. After the previous pull request, my colleague Matt Stead and I re-worked the code for determining Neo streams, and addressed all the comments from the second pull request. We also verified that multiple MED wrappers can point to the same dataset.
Please let us know if there any questions or comments! Thanks.