simple proposal for buffer_id in rawio #1544

samuelgarcia · 2024-09-03T20:21:22Z

A very simple proposal for spikeglxrawio
@h-mayorquin
@zm711

neo/test/rawiotest/rawio_compliance.py

samuelgarcia · 2024-09-03T20:23:04Z

neo/rawio/spikeglxrawio.py

+            buffer_id = stream_name
+            buffer_name = stream_name
+            signal_buffers.append((buffer_name, buffer_id))


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.

zm711 · 2024-09-03T20:37:07Z

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 ?

h-mayorquin · 2024-09-04T02:50:02Z

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:

#1545

samuelgarcia · 2024-09-04T07:24:14Z

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 ?

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.

samuelgarcia · 2024-09-04T07:26:01Z

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:

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.

h-mayorquin · 2024-09-04T19:38:50Z

Good. We covered in the discussion today:
The buffer will be undefined if:

The stream is divided across multiple files. Example intan's one-file-per-channel
There is an API between neo and the data which obfuscates the data layout. Example plexon2

We decide between two approaches:

Have a strict version where stream and buffer are orthogonal concepts.
Have a "nested-unless-edge-case" approach where the stream headers contain a reference fo their buffer unless the buffer is hard to define (cases 1 and 2 above).

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.

zm711 · 2024-09-04T20:28:54Z

Good. We covered in the discussion today:
The buffer will be undefined if:

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

plexon 1 for example is one file, but multiple memmaps.
Sam mentioned thinking about intan header-attached which is one file, one memmap but with the block structure doesn't quite fit with what he was imagining for a buffer (when you hopped off @h-mayorquin).

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!

samuelgarcia · 2024-09-05T08:35:38Z

Thanks for this summary of internal discussion @h-mayorquin and @zm711.

samuelgarcia · 2024-09-05T15:19:11Z

@zm711 @h-mayorquin this is ready on my side.

zm711 · 2024-09-05T15:53:15Z

Looks like raw binary signal wasn't done quite right (tests failing). Want to fix that? I'll look over this in parallel.

zm711 · 2024-09-05T16:44:42Z

neo/rawio/axonrawio.py

@@ -56,6 +56,7 @@
    BaseRawIO,
    _signal_channel_dtype,
    _signal_stream_dtype,
+    _signal_buffer_dtype,


sneaky. could've had them alphabetized, but that would be too conventional.

lets leave a chance to the anarchy style

zm711 · 2024-09-05T16:46:00Z

neo/rawio/blackrockrawio.py

@@ -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)


I'll be honest I don't know blackrock at all. I'll trust you on this one.

zm711 · 2024-09-05T16:48:13Z

neo/rawio/examplerawio.py

@@ -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


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.

zm711 · 2024-09-05T16:49:50Z

neo/rawio/intanrawio.py

            signal_streams["name"][stream_index] = name
+            # zach I need you help here


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.

zm711 · 2024-09-05T16:51:12Z

neo/rawio/openephysrawio.py

-            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


c'est parfait! That's a perfect explanation of why not.

zm711 · 2024-09-05T16:53:24Z

neo/rawio/tdtrawio.py


        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


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?

Yes this is what I meant

h-mayorquin · 2024-09-07T01:37:00Z

I will take a look once the tests are passing.

zm711 · 2024-09-09T12:22:24Z

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.

zm711 · 2024-09-13T14:02:59Z

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.

samuelgarcia · 2024-10-11T09:29:54Z

@zm711 : it would be nice to merge this soon.
So I can continue working on #1513.

…d_signal_buffer_id

zm711 · 2024-10-11T10:21:36Z

scanning through this it looks good. Once tests pass I'll do a final read through, so let's target to merge this by end of weekend. We can ping @h-mayorquin for a read through too, hopefully today if he has time?

zm711 · 2024-10-11T14:30:27Z

neo/test/rawiotest/test_neuronexusrawio.py

@@ -12,3 +12,7 @@ class TestNeuroNexusRawIO(
    rawioclass = NeuroNexusRawIO
    entities_to_download = ["neuronexus"]
    entities_to_test = ["neuronexus/allego_1/allego_2__uid0701-13-04-49.xdat.json"]
+
+
+if __name__ == "__main__":


zm711

This works for me. There are a couple of comment touch ups and some clean up we should do as we push forward, but I agree it is too hard to keep up the parallel structure for so long.

h-mayorquin

LGMT. I added some comments that I think will be helpful for future developers.

I still think that including the buffer in the data signal_stream might lead to problems in the future and I would prefer the concepts to be decoupled (the schema to be normalized in the database sense and make the signal channel array/table the normalized table).

But, what is important for data extraction is that we can decouple the stream and the buffer and divide the streams "logically" and this PR allows us to do this:

Fine by me.

neo/rawio/baserawio.py

neo/rawio/edfrawio.py

neo/rawio/baserawio.py

zm711 · 2024-10-11T15:49:38Z

If Sam doesn't respond I'll package up your edits into a commit and push them, then once final tests pass we can merge it.

Definitely down for another call at some point to discuss your desire for decoupling more !

zm711

adding a couple doc fixes to be added with Heberto's

neo/rawio/plexon2rawio/plexon2rawio.py

neo/rawio/examplerawio.py

Co-authored-by: Heberto Mayorquin <[email protected]>

samuelgarcia · 2024-10-14T10:10:47Z

merci les amis

simple proposal for buffer_id in rawio

be1a3c9

samuelgarcia mentioned this pull request Sep 3, 2024

neo.rawio : API enhance proposal buffer_id and stream_id #1543

Open

zm711 reviewed Sep 3, 2024

View reviewed changes

neo/test/rawiotest/rawio_compliance.py Outdated Show resolved Hide resolved

samuelgarcia commented Sep 3, 2024

View reviewed changes

h-mayorquin mentioned this pull request Sep 4, 2024

[Showcase] Enchance headers with buffer id at BaseRawIO #1545

Closed

WIP : buffer_id for many rawios

b54ec82

samuelgarcia added 3 commits September 5, 2024 15:01

wip : more rawio with buffer_ids

a9340f1

wip buffer_id for more rawios

451325c

fix tests

d08e265

zm711 reviewed Sep 5, 2024

View reviewed changes

samuelgarcia added 2 commits September 9, 2024 10:49

coments update

a977897

Fix conflict with master an plexon2

f2dd922

merge with main and resolve conflicts

72f0787

samuelgarcia added 2 commits October 11, 2024 11:09

add buffer id in neuronexus format

c31f1c7

fix plexon2rawio

b8a60c5

samuelgarcia added 3 commits October 11, 2024 11:34

fix neuronexus buffer_id

d11be08

Merge branch 'master' of github.com:NeuralEnsemble/python-neo into ad…

1552a50

…d_signal_buffer_id

oups

b864d21

zm711 reviewed Oct 11, 2024

View reviewed changes

zm711 approved these changes Oct 11, 2024

View reviewed changes

h-mayorquin approved these changes Oct 11, 2024

View reviewed changes

neo/rawio/baserawio.py Outdated Show resolved Hide resolved

neo/rawio/edfrawio.py Show resolved Hide resolved

neo/rawio/baserawio.py Outdated Show resolved Hide resolved

zm711 reviewed Oct 11, 2024

View reviewed changes

neo/rawio/plexon2rawio/plexon2rawio.py Outdated Show resolved Hide resolved

neo/rawio/examplerawio.py Outdated Show resolved Hide resolved

neo/rawio/examplerawio.py Outdated Show resolved Hide resolved

neo/rawio/examplerawio.py Outdated Show resolved Hide resolved

zm711 and others added 2 commits October 11, 2024 13:17

Apply suggestions from code review Heberto + Zach

a3bac2a

Co-authored-by: Heberto Mayorquin <[email protected]>

Merge branch 'master' into add_signal_buffer_id

fee1537

zm711 merged commit 45a363d into NeuralEnsemble:master Oct 11, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

simple proposal for buffer_id in rawio #1544

simple proposal for buffer_id in rawio #1544

samuelgarcia commented Sep 3, 2024

samuelgarcia Sep 3, 2024

zm711 commented Sep 3, 2024

h-mayorquin commented Sep 4, 2024

samuelgarcia commented Sep 4, 2024

samuelgarcia commented Sep 4, 2024

h-mayorquin commented Sep 4, 2024

zm711 commented Sep 4, 2024

samuelgarcia commented Sep 5, 2024

samuelgarcia commented Sep 5, 2024

zm711 commented Sep 5, 2024

zm711 Sep 5, 2024

samuelgarcia Sep 9, 2024

zm711 Sep 5, 2024

zm711 Sep 5, 2024

zm711 Sep 5, 2024

zm711 Sep 5, 2024

zm711 Sep 5, 2024

samuelgarcia Sep 9, 2024

h-mayorquin commented Sep 7, 2024

zm711 commented Sep 9, 2024

zm711 commented Sep 13, 2024

samuelgarcia commented Oct 11, 2024

zm711 commented Oct 11, 2024

zm711 Oct 11, 2024

zm711 left a comment

h-mayorquin left a comment

zm711 commented Oct 11, 2024

zm711 left a comment

samuelgarcia commented Oct 14, 2024

		signal_streams["name"][stream_index] = name
		# zach I need you help here

simple proposal for buffer_id in rawio #1544

simple proposal for buffer_id in rawio #1544

Conversation

samuelgarcia commented Sep 3, 2024

Choose a reason for hiding this comment

zm711 commented Sep 3, 2024

h-mayorquin commented Sep 4, 2024

samuelgarcia commented Sep 4, 2024

samuelgarcia commented Sep 4, 2024

h-mayorquin commented Sep 4, 2024

zm711 commented Sep 4, 2024

samuelgarcia commented Sep 5, 2024

samuelgarcia commented Sep 5, 2024

zm711 commented Sep 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-mayorquin commented Sep 7, 2024

zm711 commented Sep 9, 2024

zm711 commented Sep 13, 2024

samuelgarcia commented Oct 11, 2024

zm711 commented Oct 11, 2024

Choose a reason for hiding this comment

zm711 left a comment

Choose a reason for hiding this comment

h-mayorquin left a comment

Choose a reason for hiding this comment

zm711 commented Oct 11, 2024

zm711 left a comment

Choose a reason for hiding this comment

samuelgarcia commented Oct 14, 2024