spikeinterface icon indicating copy to clipboard operation
spikeinterface copied to clipboard

`set_probe` behavior for `RecordingExtractors`

Open h-mayorquin opened this issue 1 year ago • 4 comments

We were discussing on slack about set_probe behavior. I am recaping the points here in case we want to make a decision about it.

Context: Me and @magland were mentioning that we have been confused about this method. Basically, it returns a new recording with the attached probe instead (as we expected) of setting the probe in the current recording. To achieve the latter, you need to use the in_place keyword.

Now, @alejoe91 mentioned some technical difficulties that are relevant. Basically, for most probe structures, the method can't modify state unless the following condition is met:

https://github.com/SpikeInterface/spikeinterface/blob/329197618a9b48ef876d1e8b8e79f07f4abf5e49/src/spikeinterface/core/baserecordingsnippets.py#L176-L185

So, a true setter method would not be as general.

Then @samuelgarcia mentioned that, in his view, set_probe is following the logic of set_index in pandas with the in_place logic. He believes this is just a matter of documentation and not semantics.

I disagree with that argument for three reasons:

  • Even if pandas uses set it is still a bad name. Set methods are the fundamental mutator method, I don't know think they should be added in a fluent interface anyway.
  • Pandas has some problems on their own on that regard. See the SettingWithCopyWarning in stackoverflow and tutorials.
  • in_place works in whacky ways in Pandas, mantainers have expressed their desire to deprecate the keyword and I don't think we should build on that.

My own proposal would be to make the current set_probe method work like a true setter and fail when this is not possible. We could have another method then that works to build a probe independent of the structure so this always return a new recorder object. That is, in my view, is another method that should work in a fluent interface way. @magland proposed recording.create_new_recording_with_probe(probe=probe) which works very well to me.

Any other suggestions or experiences? It could be that this is very clear than most people and mine and Jeremy's troubles are statistical flukes.

h-mayorquin avatar Nov 12 '23 13:11 h-mayorquin