EEG-1200A V01.00#11431
EEG-1200A V01.00#11431jacobshaw42 wants to merge 9 commits intomne-tools:mainfrom jacobshaw42:nk1200a
Conversation
|
Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴🏽♂️ |
|
marking as draft based on this comment:
@ftadel we need permission to relicense an adaptation of some Brainstorm code (GPL-3 to BST); it's for reading Nihon-Koden files. OK for you? |
Sure. Please share this collective work as much as possible. If you are implementing support for files that are currently not supported in Brainstorm (like EEG-1200A V01.00 with multiple data blocks), could you please let me know and share an example file with me? - Thanks |
drammock
left a comment
There was a problem hiding this comment.
here's a first pass review @jacobshaw42. Thanks for implementing this! Ping us when these are addressed and you're ready for another look.
Also can you share a file with this version that we can use for testing? Apologies if you've posted the link elsewhere already and I just missed it.
Ideally we would (also) have a very short (just a few seconds) file to use in I/O unit tests. If there's a way to have it include fewer channels and still be useful for testing, that is also desirable.
| if header['version'] == _valid_headers[-1]: | ||
| self._annotations = annotations | ||
| else: | ||
| self.set_annotations(annotations) |
There was a problem hiding this comment.
I'm still a bit wary of this change and would like input from other devs. set_annotations has some sensible sanity/safety checks and I don't like the idea of skipping them; I'd rather figure out why things aren't working and fix them.
There was a problem hiding this comment.
After looking into this more closely. I believe this has to do with the multiple blocks of data. There is only one collected (due to the version), so the rest of the annotations would be seen as out of range for the data found. Skipping this would allow to get the annotations even though we have no data for those annotations.
I am not sure that is helpful to have the annotations without the observations. I will spend sometime trying to get all of those.
|
@drammock @jacobshaw42 did we discard the plan for supporting 'Nihon Kohden EEG-1200A V01.00'? |
|
Is there going to be a solution for supporting 'Nihon Kohden EEG-1200A V01.00'? |
The contributor who was working on this has deleted their fork of MNE-Python (and so presumably abandoned the effort). Without access to a sample data file (ideally a "normal" one and also a very short one for using in tests), none of our devs can pick up where they left off. So at this point we're waiting on either an example file (and one of our devs to find time), or a different contributor who has an example file and who has time now to get this over the finish line (including some basic tests). |
|
Support for 'Nihon Kohden EEG-1200A V01.00' is urgently needed. Though Neuroworkbench can convert to edf type, it is unrealistic to manually convert a large number of eeg files into edf. |
|
@wxl-001 do you have (or could you get) some small test files (e.g., 1 sec of data?) for us to test? |
I'm very sorry that I don't have the permission to share the data with others. Wish you could solve this problem as soon as possible. |
|
Can't work on it without files 😞 |
Hi @larsoner, |
Reference issue
Fixes #9462.
What does this implement/fix?
Adds compatibility for Nihon EEG-1200A V01.00 format. This involves bypassing the set annotations method for only this version.
Additional information
There is also a "sub-log" in the
.LOGfile. This adds more information to the annotations description and milliseconds to the annotation onsets.Part of the "sub-log" code was inspired by brainstorm. Due to the differences in the flow of these functions, I did not "copy" or directly translate. However, I am not sure how the licenses play in this case. If this inspiration is not compatible with licenses, please disregard my pull request.