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
322 stars 247 forks source link

Inconsistent `rescale_spike_timestamp` behavior for spikes in Plexon? #1304

Closed h-mayorquin closed 3 months ago

h-mayorquin commented 1 year ago

In the base class the docstring of the rescale_spike_timestamp function claims that it should rescale timestamps to seconds:

https://github.com/NeuralEnsemble/python-neo/blob/4d4eb85e459c7e35cc1d43ce423fd58907fb3002/neo/rawio/baserawio.py#L646-L650

However, looking at Plexon it seems that the private method is in fact returning frames?

https://github.com/NeuralEnsemble/python-neo/blob/4d4eb85e459c7e35cc1d43ce423fd58907fb3002/neo/rawio/plexonrawio.py#L352-L355

This is very confusing, because in SpikeInterface we are using the self._global_ssampling_rate as the sampling frequency of the sorting extractor so something does not square. Is there something I am missing

All the other sorting extractors look reasonable even if I don't know everything about them:

Neuralynx: https://github.com/NeuralEnsemble/python-neo/blob/4d4eb85e459c7e35cc1d43ce423fd58907fb3002/neo/rawio/neuralynxrawio/neuralynxrawio.py#L645-L649

Are the spiketimes of neuralynx naturally in microseconds?

Blacrock: https://github.com/NeuralEnsemble/python-neo/blob/4d4eb85e459c7e35cc1d43ce423fd58907fb3002/neo/rawio/blackrockrawio.py#L661-L664

I am kind of surpirsed this is not done inside the _get_spike_timestamps in blackrock as it seems that _get_spike_timestamps naturally does not return timestamps?

Anyway, concerning Plexon, am I misunderstanding? You have been working on this recently @JuliaSprenger so maybe you have this fresh?

h-mayorquin commented 1 year ago

Oh, after thinking more about this I think the behavior above is the correct one.

The central conusion comes from IMHO the very confusing fact that _get_spike_timestamps does NOT always return timestamps... so the function _rescale_spike_timestamp is a patch in top of that behavior.

So, am I correct that plexon _get_spike_timestamps function returns frames?

h-mayorquin commented 1 year ago

OK, I am writing this for me and somebody who reads in the future.

Another confusing thing is that it would be confusing enough if there were two behaviors with an ambiguity for _get_spike_timestamps: 1) Sometimes it returns timestamps in seconds 2) Sometimes it returns frames

But it seems that there are three behaviors? 1) Sometimes it returns timestamps in seconds. 2) Sometimes it returns timestamps in microseconds (neuralynx) 3) Sometimes it returns frames.

This is even without taking into account the frame reference for which I know less but I have a sense there is some further variability there (is the 0 in the frames with respecto to what? is the t_start in the timestamps with respect to what).

zm711 commented 6 months ago

@h-mayorquin,

Just reading this now, but my understanding is that yes the rescaling is a patch on top in that get_spike_timestamps returns whatever the file format returns (sometimes seconds, microseconds, frames). Then regardless of what get_spike_timestamps returns we force the return in seconds with rescale_spike_timestamps. It is similar to get_analogsignal_chunk which returns it will the dtype of the file format and then you have to rescale to use the gain and offset to get voltages. If you see something that looks super inaccurate/wrong though I would open a specific issue to make sure whether or not we need to patch this.

zm711 commented 3 months ago

Want to ping @h-mayorquin for this. Do you want more discussion around this? Or are we okay to close.

Again simple answer is:

get_timestamps: returns the raw data in whatever units it is rescale_timestamps: is suppose to ensure everything is in seconds.

h-mayorquin commented 3 months ago

No, I am closing this. I still think the name and return semantics of the function is strange and there might be something worong. But back then fixed this at Spikeinterface by using class attributes at the level of NeoBaseSortingExtractor.

https://github.com/h-mayorquin/spikeinterface/blob/1be4e7e64f2b6a852b72561513b7dfa1dcc2e6b6/src/spikeinterface/extractors/neoextractors/neobaseextractor.py#L351-L365

I don't have to delve into this now so I am closing.