simple proposal for buffer_id in rawio #1544
Conversation
| buffer_id = stream_name | ||
| buffer_name = stream_name | ||
| signal_buffers.append((buffer_name, buffer_id)) |
There was a problem hiding this comment.
At the moment the mapping buffer to stream is one to one.
But after this PR we will be able to split buffer into logical stream super easily.
|
In your example the idea would be that we move the sync channel into a new stream right? So it will keep the buffer but you'll assign a new stream ? |
|
Hey, Sam, what I had in mind last time we discussed was something like this that I think should be less concentrate the disruption in one PR and then we can work piece by piece on the formats: |
In this PR the idea to have no change IOs. Only adding this buffer_id stuff in a very neutral way. The next PR will have some changes for some IOs (plexon, spikeglx, ced) to split actual stream in more streams but with the same buffer. |
Your proposal is cool and easy but I think I prefer to be brave and explicitly add this features everywhere as a necessary implementation for rawios. |
|
Good. We covered in the discussion today:
We decide between two approaches:
I am weary of not going with the strict apporach but this will faciliate some work on Sam and I could not find a concrete example where this could cause problems. In such a case "practicallity should beat purity" and we will stick to this. |
I think what I would add to this so we don't forget that we didn't completely settle on a rigid buffer definition so that we have some flexibility as we implement it. Buffers ought to have same dtype, sampling rate, but something like same file vs same memmap was left in the air
Thanks for summary of the discussion @h-mayorquin! I think keeping our documentation up will help us when we go back and think about our choices! |
|
Thanks for this summary of internal discussion @h-mayorquin and @zm711. |
|
@zm711 @h-mayorquin this is ready on my side. |
|
Looks like raw binary signal wasn't done quite right (tests failing). Want to fix that? I'll look over this in parallel. |
| @@ -56,6 +56,7 @@ | |||
| BaseRawIO, | |||
| _signal_channel_dtype, | |||
| _signal_stream_dtype, | |||
| _signal_buffer_dtype, | |||
There was a problem hiding this comment.
sneaky. could've had them alphabetized, but that would be too conventional.
There was a problem hiding this comment.
lets leave a chance to the anarchy style
| @@ -399,7 +401,10 @@ def _parse_header(self): | |||
| ext_header.append(d) | |||
|
|
|||
| if len(ext_header) > 0: | |||
| signal_streams.append((f"nsx{nsx_nb}", str(nsx_nb))) | |||
| buffer_id = stream_id = str(nsx_nb) | |||
There was a problem hiding this comment.
I'll be honest I don't know blackrock at all. I'll trust you on this one.
| @@ -111,12 +112,18 @@ def _parse_header(self): | |||
| # In short `_parse_header()` can be slow but | |||
| # `_get_analogsignal_chunk()` needs to be as fast as possible | |||
|
|
|||
| # create fake signal streams information | |||
| # create fake signal streams and buffer information | |||
| # a buffer is a group of channels that are in the same buffer for instance hdf5 or binary : this is optional | |||
There was a problem hiding this comment.
I'm wondering if we want to weird this differently. buffer is not optional in the developer sense because it has to be a value or "" but is optional in the sense that you can give an "" string. So maybe we make that clearer. Let me think about this while you fix the tests.
| signal_streams["name"][stream_index] = name | ||
| # zach I need you help here |
There was a problem hiding this comment.
I think we need to say why we don't count these as buffers.
So for header-attached it is because the data is set up in blocks so there is no continous signal
and for one-file-per-channel it is because the stream is split among files.
| signal_streams = np.array([]) | ||
| signal_streams = [] | ||
| signal_streams = np.array(signal_streams, dtype=_signal_stream_dtype) | ||
| # no buffer handling in this format because one channel per file |
There was a problem hiding this comment.
c'est parfait! That's a perfect explanation of why not.
neo/rawio/tdtrawio.py
Outdated
|
|
||
| if missing_sev_channels: | ||
| warnings.warn(f"Could not identify sev files for channels {missing_sev_channels}.") | ||
|
|
||
| signal_streams = np.array(signal_streams, dtype=_signal_stream_dtype) | ||
| signal_channels = np.array(signal_channels, dtype=_signal_channel_dtype) | ||
| # buffer concept here, data are spread per channel and paquet |
There was a problem hiding this comment.
Qu'est-ce que c'est un paquet la? channel are files and the paquet is for folder? or packets of info meaning like internal blocks of data like data packets?
There was a problem hiding this comment.
Yes this is what I meant
|
I will take a look once the tests are passing. |
|
Now plexon2 tests are failing. Heberto has done a bunch of updates. He has another PR that we need to review (but I need to check if it is plexon1 or plexon2). Do we want to get his last PR done and then you can finalize this or do you want to finish this first. There are slightly organizational changes occurring that I think will keep leading to test failures. Up to you both from my perspective. |
|
I think with the latest boost to plexon2 once you fix conflicts and merge from main I hope the plexon2 won't give us any troubles. |
A very simple proposal for spikeglxrawio
@h-mayorquin
@zm711