OpenEphysBinaryRawIO: Separate Neural and Non-Neural Data into Distinct Streams
#1624
Conversation
|
Testing data is here: https://gin.g-node.org/NeuralEnsemble/ephy_testing_data/pulls/150 Once the data is merged I can add a test |
|
@h-mayorquin can you fix Ryan's last name here? https://gin.g-node.org/NeuralEnsemble/ephy_testing_data/pulls/150#issuecomment-5561 Se we can merge and add a test here |
neo/rawio/openephysbinaryrawio.py
Outdated
| if stream_index >= len(self._sig_streams[block_index][seg_index]): | ||
| stream_index = stream_index - len(self._sig_streams[block_index][seg_index]) | ||
| t_start = self._sig_streams[block_index][seg_index][stream_index]["t_start"] |
There was a problem hiding this comment.
I understand correctly the ptach stream_index is not anymore a stream_index but an internal variable which is not a direct mapping to stream_index.
self._sig_streams[block_index][seg_index][stream_index]["t_start"]
If this is the case I would change this internal variable to something else to avoid mistale when reading the entire code.
Maybe I am mistunderstanding or not clear at all but this writting is a bit weird to me.
There was a problem hiding this comment.
You are correct. This is a good idea.
Co-authored-by: Alessio Buccino <[email protected]>
|
I corrected Ryan's last name (my bad) on the test data repo. Question: why do we have stream_id of the synch channel as the empty string? It is loaded with the neural data of the continuous event with a flag, shouldn't they belong to the same stream? |
|
Gin is merged! |
|
@h-mayorquin so just missing the test with the new test file and this is ready for final review, right? |
|
This is ready for final review now. |
neo/rawio/openephysbinaryrawio.py
Outdated
| stream_id_neural = stream_id | ||
| stream_id_non_neural = str(int(stream_id) + self._num_of_signal_streams) | ||
|
|
||
| # Note this implementation assumes that the neural channels come before the non-neural channels |
There was a problem hiding this comment.
Here guys, these are the strongest assumptions that the reader makes. I wanted to check with @alejoe91 if he knows about this:
- Is it true that the synch trace will always be the last channel even if you have non-neural channels? I could not find this in the documentation.
- Are the neural channels and non-neural always in order and not interleaved?
If the first case is not True, we can extract the specific index from the structure.oebin. That's a bigger change but I think doable.
For the second case, if it is not true, I am afraid we will have to error somehow as the current data model does not support non-contigious streams across the buffer with the buffer slice mechanism (@samuelgarcia )
There was a problem hiding this comment.
I am adding error messages for this advising people to open an issue if they have interleaved neural and non-neural or synch trace that is not at the end.
|
@h-mayorquin some tests are failing. This is because the array annotations have changed now, since also they need to be split across streams |
I fixed them now. The array annotations were not divided for the stream. |
|
@alejoe91 tests are passing but the docs are failing for some reason (they have failed intermittently through this PR) not sure if related to this. Updating the branch to check. |
neo/rawio/openephysbinaryrawio.py
Outdated
| # For ADC channels multiplying by the bit_volts when units are not provided converts to Volts | ||
| units = "V" if units == "" else units | ||
|
|
||
| gain = chan_info["bit_volts"] |
There was a problem hiding this comment.
is this a safe float? or do we need to convert it?
There was a problem hiding this comment.
What do you mean safe? as in numpy?
There was a problem hiding this comment.
Yes. I mean is it a numpy scalar (I should have written that better :) ) or is it a python float?
There was a problem hiding this comment.
For floats, there is no difference I think right? We only have to be concerned about integers. I think np.float is the same thing as float in python (from my memory).
There was a problem hiding this comment.
I changed it anyway I guess null casting should have no cost.
There was a problem hiding this comment.
So I looked it up mostly from stackoverflow discussion
https://stackoverflow.com/questions/40726490/overflow-error-in-pythons-numpy-exp-function
and reddit
https://www.reddit.com/r/learnpython/comments/feakn2/numpy_overflow_help_needed/?rdt=48979
And seems like it can overflow, but honestly the issue of float overflow seems to be more benign and casting to python seems to cost precision that honestly shouldn't really matter right? But float precision is so OS dependent that I think requiring that level of precision can't really be expected from floats. Appreciate the casting though, but in general your intuition/memory of this being an int thing seems to be true.
| for stream_index, stream_name in enumerate(sig_stream_names): | ||
| # stream_index is the index in vector sytream names | ||
| # stream_index is the index in vector stream names |
neo/rawio/openephysbinaryrawio.py
Outdated
| # We defined their stream_id as the stream_index of neural data plus the number of neural streams | ||
| # This is to not break backwards compatbility with the stream_id numbering | ||
| stream_id = str(stream_index + len(sig_stream_names)) | ||
| # For ADC channels multiplying by the bit_volts when units are not provided converts to Volts |
There was a problem hiding this comment.
What do you mean here?
the way it is written sounds like if units are provided then we SHOULD NOT worry about using the gain,? So is the gain 1 in this case and our code is fine?
There was a problem hiding this comment.
Yes, that's not very clear without context. More context: When the units are not provided the units of ADC are Volts. This is in opposition to the neural channels where if the units are not provided the units are microvolts.
https://open-ephys.github.io/gui-docs/User-Manual/Recording-data/Binary-format.html#continuous
But the units are always the units of the data AFTER you use the gain. Only the unitless(units=="") case differs between neural and non-neural channels. Microvolts for the former, volts for the latter.
There was a problem hiding this comment.
Okay, yeah that makes sense in the context (and is the same with intan so ADC being volts and neural being microvolts seems common enough.
There was a problem hiding this comment.
Re-wrote the whole thing to separate unit determination from stream determination and I think it reads better now.
There was a problem hiding this comment.
Yeah I agree it makes it clearer to me at least!
The continuous.dat file in the Open Ephys Binary format often contains both neural and non-neural (ADC) channels. For detailed documentation, refer to the Open Ephys GUI User Manual.
An example dataset (provided by @rly) illustrating this structure is available here. Additionally, a related discussion can be found in the NeuroConv repository: Issue #1023. I think this is the module that the test set used
This pull request modifies the reader to separate non-neural channels into their own stream. This approach allows users to clearly distinguish between neural and non-neural data and will enable programmatic separation in downstream libraries like SpikeInterface and Neuroconv.
Implementation Details:
To maintain backward-compatibility of stream_ids, the non-neural data's stream_id is defined as the neural data's stream_id plus the total number of available streams. For example, if the neural data has a stream_id of "1" and there are three streams in total, the non-neural data will be assigned a stream_id of "4".
The stream_name for non-neural data is derived by appending _ADC to the neural data's stream_name. For instance, if the neural data's stream_name is Data, the non-neural data's stream_name will be DATA_ADC.
I welcome better suggestions for conventions of naming and any other type of feedback from people who are more familiar with this format @alejoe91