Deprecate cast_unsigned as an argument of core functions?

[h-mayorquin]

In get traces we have the cast_unsigned parameter:

spikeinterface/src/spikeinterface/core/baserecording.py

Lines 234 to 243 in f779e12

	def get_traces(
	self,
	segment_index: Union[int, None] = None,
	start_frame: Union[int, None] = None,
	end_frame: Union[int, None] = None,
	channel_ids: Union[Iterable, None] = None,
	order: Union[str, None] = None,
	return_scaled=False,
	cast_unsigned=False,
	):

Which I think is just another way of doing what @alejoe91 enabled with a preprocessing step on:
#1707

We also have a related parameter auto_cast_uint in write_binary_recording

spikeinterface/src/spikeinterface/core/core_tools.py

Lines 244 to 252 in f779e12

	def write_binary_recording(
	recording,
	file_paths=None,
	dtype=None,
	add_file_extension=True,
	byte_offset=0,
	auto_cast_uint=True,
	**job_kwargs,
	):

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?

• Maintenance of unsigned to sign casting: having this logic in a centralized place (the UnsignedToSignedRecording class) means is easier to debug, test and document.
• Discoverability: It is easier to discover a form of preprocessing in the preprocessing module that to find it across flags spread around different functions.
• Maintenance of core: I have the firm belief that the core functions should be a simple as possible. One form of simplicity is signature simplicity. Having flags in core functions to resolve what are preprocessing or IO use cases increases drastically the surface area for bugs to sneak in. The branching and the number of cases to consider increases as the use is spread all over the library and is harder to reason about changes. It increases testing time if we want to be complete. The philosophy is keep the core simple and export complexity to the use cases.

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]

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]

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]

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