SpikeInterface / spikeinterface

A Python-based module for creating flexible and robust spike sorting pipelines.
https://spikeinterface.readthedocs.io
MIT License
535 stars 188 forks source link

Deprecate `cast_unsigned` as an argument of core functions? #1815

Open h-mayorquin opened 1 year ago

h-mayorquin commented 1 year ago

In get traces we have the cast_unsigned parameter:

https://github.com/SpikeInterface/spikeinterface/blob/f779e12191515b49ff4f73a02d453f44e1be00ec/src/spikeinterface/core/baserecording.py#L234-L243

Which I think is just another way of doing what @alejoe91 enabled with a preprocessing step on: https://github.com/SpikeInterface/spikeinterface/pull/1707

We also have a related parameter auto_cast_uint in write_binary_recording

https://github.com/SpikeInterface/spikeinterface/blob/f779e12191515b49ff4f73a02d453f44e1be00ec/src/spikeinterface/core/core_tools.py#L244-L252

So, we have this functionality that I think can be achieved with the preprocessing class UnsignedToSignedRecording. My naive take is this should be slowly deprecated in favor of using this transformation as a preprocessing step when required.

Why?

I really don't see an advantage to preserving this but maybe I lack imagination? What I am not seeing? I undestand why they came to be there but now that we have a preprocessing module to accomplish that it seems like the way to go.

Thoughts?

alejoe91 commented 1 year ago

Hi @h-mayorquin

I agree that we could slowly deprecate both. IMO we should raise an exception (after deprecation) in the write_binary_recording if the dtype is uint and dtype is int. We should do the same in the filter functions, since there is a mechanism there to automatically cast to integer too

h-mayorquin commented 1 year ago

In write_binary_recording, what is the use case for specifying the dtype instead of using the dtype of the recording that is to be saved?

It seems to me that we should always use the dtype of the recording. Now we have the AstypeRecording preprocessing which people can use to explicitliy cast if that's what they want.

alejoe91 commented 1 year ago

Yes. That is very convenient when writing int16 from preprocessed floats before running sorters. I agree that making an astype first is better