NeuralEnsemble / python-neo

Neo is a package for representing electrophysiology data in Python, together with support for reading a wide range of neurophysiology file formats
http://neo.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
325 stars 248 forks source link

simple proposal for buffer_id in rawio #1544

Closed samuelgarcia closed 1 month ago

samuelgarcia commented 2 months ago

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

zm711 commented 2 months ago

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 commented 2 months ago

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:

https://github.com/NeuralEnsemble/python-neo/pull/1545

samuelgarcia commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

Good. We covered in the discussion today: The buffer will be undefined if: 1) The stream is divided across multiple files. Example intan's one-file-per-channel 2) There is an API between neo and the data which obfuscates the data layout. Example plexon2

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.

zm711 commented 2 months ago

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

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 commented 2 months ago

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

samuelgarcia commented 2 months ago

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

zm711 commented 2 months ago

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

h-mayorquin commented 2 months ago

I will take a look once the tests are passing.

zm711 commented 2 months ago

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 commented 2 months ago

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 commented 1 month ago

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

zm711 commented 1 month ago

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 commented 1 month ago

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 !

samuelgarcia commented 1 month ago

merci les amis