Separate sync as its own stream in OpenEphysBinaryRawIO

[h-mayorquin]

With love for @chrishalcrow

As discussed with @alejoe91 and @samuelgarcia

This PR:

• Keeps backward compatibility for load_sync_trace=True but throws a deprecation warning (what is neo deprecation policy @zm711?)
• Adds the sync channels as their own stream so they can be loaded by downstream libraries.
• Adds a tests.

[chrishalcrow]

Hello, great! Just for reference: the main application of this is to be able to let NeuroConv deal with sync channels in a nice way. And so that loading sync channels in spikeinterface doesn't mess up the probe object.

One possible problem: with this PR in using read_openephys with spikeinterface you'd now have to specify your steam. E.g. if you just do a vanilla

rec = si.read_openephys('path/to/recording')
>>> ValueError: This reader have several streams: 
Names: [np.str_('Record Node 109#Neuropix-PXI-103.ProbeA'), np.str_('CH_SYNC')]
IDs: [np.str_('0'), np.str_('CH_SYNC')]. 
Specify it from the options above with the 'stream_name' or 'stream_id' arguments

whereas before, there was only one stream, so it just used that. So this will break code for people using openephys with just neural and sync streams (e.g. my lab!). Is there a simple way (on the spikeinterface side?) to retain the current default behavior?

EDIT: oh, actually making si ignore the sync channel when load_sync_channel is false is easy...

[guptadivyansh]

This would also have implications for #1657. In that PR I'm extracting sync edges from SpikeGLX recordings as events, rather than exposing the entire sync stream that is being done here.

I don't have a preference for one or the other but I do think the handling/API should be consistent across SGLX and OpenEphys.

I can make changes to the other SGLX PR if the extra sync stream has already been agreed upon.

[zm711]

I think users of this IO would need to provide more feedback to us. I don't use this so I also don't really know the best solution. I think Sam and Heberto have talked the most about this. So maybe Heberto you could summarize more here why you think the sync should be this way.

[h-mayorquin]

This would also have implications for #1657. In that PR I'm extracting sync edges from SpikeGLX recordings as events, rather than exposing the entire sync stream that is being done here.

It is already exposed there as well. Just as an extra channel when load_sync_channel=True.

I don't have a preference for one or the other but I do think the handling/API should be consistent across SGLX and OpenEphys.

I think it is fine if we do both. People can extract the synch channel as the TTL analog stream and also as rising and falling event times if they prefer it that way.

whereas before, there was only one stream, so it just used that. So this will break code for people using openephys with just neural and sync streams (e.g. my lab!). Is there a simple way (on the spikeinterface side?) to retain the current default behavior?

That's a good point. We can definitley add code in SpikeInterface to keep this from failing by checking that if there are only two streams and one of them has sync on their name then load the non-sync stream to keep backwards compatibility. The code will be ugly but we can slowly deprecate that.

[zm711]

I'm going to leave Sam or Alessio to make final decisions on this since they know this IO better than I do. I put in the request so they can get to this and contribute to the decisions.

[zm711]

More gross np.str_....

[h-mayorquin]

Fixed. How should we deprecated the load_sync_channel behavior? What is your deprecation policy here?

Removing that will simplify the code.

[alejoe91]

I would keep it for this release and change it in the next one :)

[h-mayorquin]

OK. So I will add a deprecation warning for the next release.

[h-mayorquin]

Added the deprecation.

[h-mayorquin]

I also did the equivalent PR for spikeglx #1683