BlackrockSortingExtractor - load only .nev files, ignore nsX

[luiztauffer]

There are cases when we'd like to load only the .nev file containing the events data, and ignore the local .nsX files. One example for this is when an experimenter pauses and then resumes the recording. This creates a different number of segments between nev and nsX, which breaks neo reader.
Thankfully, neo already allows for it, all we need to do is to explicitly pass the nsx_to_load argument. So, in this PR I'm proposing adding it as an optional argument to BlackrockSortingExtractor.

Ps.: It's probably be a good idea tho eventually allow passing forward all kwargs available at the neo extractors directly from the SI wrappers __init__ methods, a neo_kwargs: dict or something

[luiztauffer]

@alejoe91 @h-mayorquin could you take a look at this whenever you get a chance? thanks!

[h-mayorquin]

Can you describe more about the files? I am not familiar with this format.
I am reading you correctly that at the moment we can load events as a sorting extractor?

[luiztauffer]

@h-mayorquin
The Blackrock systems store the ecephys recordings as multiple files, e.g.:

• ns2 for LFP signals
• ns6 for high-pass filtered signals
• nev for events, including spiking units

neo can read from all of them separately, but the default configuration of parameters tells neo to read from all files together. All we’re doing here is adding to the SI wrapper a neo reader arg, that already exists but just wasn’t available to the wrapper. This new argument allows us to read only from nev, if that’s what the user wants. This is relevant for a particular lab we're working with right now.

important: this change depends on the current master branch of neo, so maybe we should push for a release over there before merging this?

[alejoe91]

Thanks @luiztauffer

I wonder if the sorting extractor should automatically just load the nev file only. The other files shouldn't be required for loading spike trains only. What do you think?

[luiztauffer]

@alejoe91 in the current implementation, the Extractor seems to depend on the streams recordings to extract the sampling rate, otherwise the user need to provide the sampling rate at instantiation.

spikeinterface/src/spikeinterface/extractors/neoextractors/neobaseextractor.py

Lines 417 to 418 in d7cf286

	if sampling_frequency is None:
	sampling_frequency = self._infer_sampling_frequency_from_analog_signal(stream_id=stream_id)

This shouldn't be the case, though. I can look into it, and see if we could make them independent.

Probably this mixing comes originally from the fact that:

• if you have nev ou probably have ns6 as well
• neo tries to read it all together at once

[luiztauffer]

@alejoe91 check it out now, let me know what you think

[h-mayorquin]

important: this change depends on the current master branch of neo, so maybe we should push for a release over there before merging this?

which feature that we merge recently? There will be release soon.

I wonder if the sorting extractor should automatically just load the nev file only. The other files shouldn't be required for loading spike trains only. What do you think?

Agree with this.

@alejoe91 the tests are failing because of pinnin numcodes. Oh, I see you merged already.

[luiztauffer]

@h-mayorquin I believe it's this one: NeuralEnsemble/python-neo@b87b8f2

[h-mayorquin]

@h-mayorquin I believe it's this one: NeuralEnsemble/python-neo@b87b8f2

Ah, the overflow of Thinh. Yes, there will be a release soon according to the neo team.

[h-mayorquin]

@luiztauffer Ok, this is hardcoded here on the python neo side anyway.

https://github.com/NeuralEnsemble/python-neo/blob/be8e663e5912dcab70d5c87a9ade80f576681702/neo/rawio/blackrockrawio.py#L301

https://github.com/NeuralEnsemble/python-neo/blob/be8e663e5912dcab70d5c87a9ade80f576681702/neo/rawio/blackrockrawio.py#L253

So no need to override the function at all. If we are going to use the wf we might as well just hardcoded in the init of blackrock and avoid all the coding complications.

[luiztauffer]

@h-mayorquin right, nice catch! Yeah, from the SI wrapper side, I believe it would be better to wait for your PR to get merged and then we can retrieve the sampling rate from the neo reader, instead of hard coding it here again. This way we keep true to whatever neo says, including if there's any future changes to that parameter

[h-mayorquin]

@alejoe91

If we consider wf_sampling_rate from the Blackrock headers a reliable heuristic for determining the sorter's sampling frequency (@samuelgarcia maybe knows better), I suggest the following path forward:

1. Expose this value as a class attribute on the Neo side, so users don’t need to parse the headers to access the main sampling frequency. See: Blackrock make sampling rate a class attribute. NeuralEnsemble/python-neo#1685

2. Add a keyword argument to BlackrockSortingExtractor like use_main_blackrock_frequency: bool = True, and pass the main sampling frequency from the header here:
   

   spikeinterface/src/spikeinterface/extractors/neoextractors/blackrock.py

   Lines 100 to 105 in 4072c3c

   	NeoBaseSortingExtractor.__init__(
   	self,
   	sampling_frequency=sampling_frequency,
   	stream_id=stream_id,
   	stream_name=stream_name,
   	**neo_kwargs,

This approach has a few advantages:

• No need to override any functionality.
• Clearly communicates the heuristic being used.
• Couples the logic to Neo, so improvements in Neo automatically propagate here.
• Provides users with an escape hatch in case the heuristic doesn’t apply in their case.

[luiztauffer]

maybe instead of use_main_blackrock_frequency: bool we could use sampling_frequency: Optional[float] = None

[h-mayorquin]

maybe instead of use_main_blackrock_frequency: bool we could use sampling_frequency: Optional[float] = None

You mean that if the sampling_frequency is None then the main sampling frequency is used?
The problem with that is it blocks the path from the other heuristic that we have that you already touched upon:

_infer_sampling_frequency_from_analog_signal

I suggest that for your conversion you should just set the sampling frequency to this value there and ask them if that the is the sampling frequency of the signal that they sorted.

[luiztauffer]

It looks like Blackrock won’t need _infer_sampling_frequency_from_analog_signal at all, or am I missing something?

If we want to keep having sampling_frequency as an optional argument for SI extractor to override whatever neo has in place, ok.

But I don’t think it’s reasonable for a Neuroconv DataInterface to have a required argument for something which is a fixed attribute down the line, in the neo reader (and it seems to be correct for that system). And I’m not even sure if leaving this as an optional override for NWB conversions would be recommended.

[h-mayorquin]

But I don’t think it’s reasonable for a Neuroconv DataInterface to have a required argument for something which is a fixed attribute down the line, in the neo reader (and it seems to be correct for that system). And I’m not even sure if leaving this as an optional override for NWB conversions would be recommended.

[bold mine]

Yes, I think that’s the core of the issue. The Neo sampling frequency is undocumented, and the last time I discussed the extractor with Sam, he mentioned he hasn’t touched it in a while. I also asked him about using the frequency from the spike header, and he said it might not be accurate. In other words, I don’t fully trust that value, and I’d prefer to leave an escape valve in case we're wrong so we don’t have to tell users to wait for another PR to correct the mistake.

It looks like Blackrock won’t need _infer_sampling_frequency_from_analog_signal at all, or am I missing something?

If there’s only one signal channel, the heuristic gets the sampling frequency from that channel, which should be the signal that was sorted. Otherwise, if there’s more than one signal, the user gets an error telling them to set the sampling frequency manually.

Regardless of all that, it's important to point out that this is a problem for us (people who work with other people's data) because we often don’t know the correct sampling frequency. Hopefully, most users will be the ones who generated the data (fingers crossed), in which case it's better if they always set the frequency themselves. Maybe in this case we should have an especial error for when the segments in the sorting are not the same as the signal which was the initial problem, right?

I’ve come around to agree with Sam that users should be responsible for setting the sampling frequency as neo is not the right tool to fetch this from and he, reasonably, does not want to maintain that. All that said, I’m okay with adding the heuristic you’re suggesting here for the sake of expediency and give the user more options. Let’s see what others think.

[luiztauffer]

just adding now the option to to pass two relevant kwargs to the neo reader: nsx_to_load and sampling_frequency
which will be used by Neuroconv's Blackrock data interface: catalystneuro/neuroconv#1290

[h-mayorquin]

+1 on this.
Will allow users to specify the sampling frequency and to load data when there are more than one segment on the recording but not in the sorting.

[h-mayorquin]

the typing of this seems strange, can you double check @luiztauffer?

Sam is going to vacations so if he does not have time to check it before that I will merge this next Monday but just double check this.

[luiztauffer]

@h-mayorquin this comes from here: https://github.com/NeuralEnsemble/python-neo/blob/c425a529958d582919607a0eb940154a4b5c4d7e/neo/rawio/blackrockrawio.py#L97

[h-mayorquin]

Ok, makes sense.