Skip to content

Conversation

@samuelgarcia
Copy link
Contributor

No description provided.

@pep8speaks
Copy link

Hello @samuelgarcia! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 27:1: W293 blank line contains whitespace
Line 30:1: W293 blank line contains whitespace
Line 33:1: W293 blank line contains whitespace

@samuelgarcia
Copy link
Contributor Author

See #654

@samuelgarcia
Copy link
Contributor Author

@apdavison @JuliaSprenger

Here I propose that OpenEphysIO will become OldOpenEphysIO
And then use OpenEphysIO for the new format.

Note that it is not a new version of the format but a total refactor since 2017.

Having the 2 formats in one class will make it super uggly and hard to maintain.
We can have a format guesser but not not at class level in a funxction it could be ok.

I prefer to have the the name OpenEphysIO for the new format.

It will make test transition difficults because I will also need to modify folder name at gin gnode.

Please if your are unhappy with the naming tell me quickly.

@bendichter
Copy link
Contributor

Is this the format of the new neuropixel OpenEphys recordings?

@samuelgarcia
Copy link
Contributor Author

Hi Ben.
This PR will implement this format (which is basically a raw format + json header):

The old class was implemented this one:

I am not aware of a specific format for NP in the OpenEphys suite.
I don't have the device, I think it was also the flat binary.

Many people use spikeglx for NP. I have implemented it here : #896

If what you mention is something else could send a link, please ?

@bendichter
Copy link
Contributor

@samuelgarcia Thanks. yes, SpikeGLX is often used, but OpenEphys also has NP capability which is being used by some labs. I'll investigate their data types and see if it matches what you have listed.

@samuelgarcia
Copy link
Contributor Author

In my mind OpenEphys can record NP with the new board pci.
This new board it is not public yet (only for the consortium, I am not in).
And the format for this is the same as a stanard openephys format if record by OE.
You should ask to @jsiegle or @aacuevas.

@JuliaSprenger JuliaSprenger changed the title [wip] implement new OpenEphyIO class for the new format. [wip] implement new OpenEphysIO class for the new format. Nov 25, 2020
@JuliaSprenger
Copy link
Member

JuliaSprenger commented Nov 25, 2020

@samuelgarcia Sorry for the late feedback. I am not sure if 'old' and 'new' are good ways to distinguish the two versions. Maybe it would be better to use a postfix based on the version used by OpenEphys?

@jsiegle
Copy link

jsiegle commented Nov 25, 2020

Hi all, just to offer some points of clarification:

  • The "original" Open Ephys format is a custom format that stores a separate data file for every channel. It's still in use by many labs, so it's important to keep support for it within Neo. However, it doesn't work for storing Neuropixels data because writing to hundreds (or even thousands) of files simultaneously is not practical. We have some updated documentation here, which may be helpful to take a look at.

  • The "new" Binary format is more of a specification for structuring directories, because the actual files are all in standard formats—flat binary, numpy, and JSON. This is the recommended format for writing Neuropixels data, but it can be used for any data source. The full specification is described here.

I would recommend keeping the OpenEphysIO name for the original format, since that's how we refer to it within the software and in the documentation. Then you can create a new class like OpenEphysBinaryIO or OEBinaryIO for the binary format.

@samuelgarcia
Copy link
Contributor Author

Thank Josh and Juilia for the feedback on naming.
I will start another PR so.

@bendichter
Copy link
Contributor

bendichter commented Jan 7, 2021

@samuelgarcia did you ever start that other PR on this? We need a RecordingExtractor for this format in the next few months. I know you and @alejoe91 have discussed moving the reading APIs to neo. I'm just trying to figure out what has been done here so we don't duplicate effort

@samuelgarcia
Copy link
Contributor Author

About effort on porting reader to neo:
we did a small hackton in december.
We have done: mearec, tridesclous, spykingcicus, and phy. #925, #926, #927, #928.

About openephys:
I started work on this a bit.
Finally on last neo hackton @JuliaSprenger start working on it.
I don't know if she made a PR.
Julia did you ?

@JuliaSprenger
Copy link
Member

Yes, I started looking into this but there is no running version yet. I hope to finish this soon, unless someone else is interested in digging into this.

@alejoe91
Copy link
Contributor

alejoe91 commented Jan 7, 2021

@JuliaSprenger I can help :)

@samuelgarcia
Copy link
Contributor Author

@alejoe91 : you don't time for this

@JuliaSprenger
Copy link
Member

Then maybe you can tell me if my change in approach makes sense to you. At first I started parsing the folders and files, like @samuelgarcia did in his first approach, but effectively I think it's better to use the structural files to do everything required for parsing the headers etc and only possibly validate later on that this corresponds to the actual folders & files being present. @alejoe91 Or do you have a better approach in mind?

@alejoe91
Copy link
Contributor

alejoe91 commented Jan 7, 2021

Hi @JuliaSprenger

I think that using the .oebin file is the best approach. It also carries information about the folders & files. Let me know if you need help

@JuliaSprenger
Copy link
Member

@alejoe91 If you know of any code that can be used for the validation part, that would be helpful.

@alejoe91
Copy link
Contributor

alejoe91 commented Jan 7, 2021

What kind of validation do you have in mind? Wouldn't running neo tests be sufficient?

@JuliaSprenger
Copy link
Member

No, I was thinking about a validation that compares the infos in the oebin with the actual data files....

@alejoe91
Copy link
Contributor

alejoe91 commented Jan 7, 2021

No, I was thinking about a validation that compares the infos in the oebin with the actual data files....

I see. To start with the number of channel and frames should be consistent. I don't know what else you could check for..I'll comment if I come up with other ideas

@bendichter
Copy link
Contributor

@JuliaSprenger any progress on this? We are trying to convert some binary open ephys data and would like to read it via neo.

@samuelgarcia
Copy link
Contributor Author

started again here : #945

@samuelgarcia samuelgarcia deleted the new_openephys_binary branch March 9, 2022 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants