`set_probe` behavior for `RecordingExtractors`

[h-mayorquin]

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:

spikeinterface/src/spikeinterface/core/baserecordingsnippets.py

Lines 176 to 185 in 3291976

	# create recording : channel slice or clone or self
	if in_place:
	if not np.array_equal(new_channel_ids, self.get_channel_ids()):
	raise Exception("set_probe(inplace=True) must have all channel indices")
	sub_recording = self
	else:
	if np.array_equal(new_channel_ids, self.get_channel_ids()):
	sub_recording = self.clone()
	else:
	sub_recording = self.channel_slice(new_channel_ids)

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.

[alejoe91]

Hi @h-mayorquin

Thanks for writing this up. Fine for me to have the set_probe/probegroup throw an error in case it can't set in place and define another function to return a new object with probe attached. I agree this would be way clearer for the end user!

[magland]

Thanks @h-mayorquin

I would revise and simplify my recommendation to a static method on BaseRecording

BaseRecording.create_with_probe(recording, probe=probe)

[magland]

And one other idea...
If you do change set_probe() to mutate rather than return a new object, then I think it would be important to NOT return a copy of the recording... because if someone is doing R = rec.set_probe(...) using the old system, then we want that to raise an Exception rather than silently mutating rec.

[JoeZiminski]

agree with this!! Thanks for raising, I have spent quite a long time stuck tracking down a bug that was caused by my misuse of the set_probe function, having expected it to make an in-place change.

Also agree with @magland that this should not return a probe due to unintended consequences. I guess just None. Most people will have something like:

recording = recording.set_probe(...)

Unfortunately this change will cause a strange and possibly quite confusing error as following lines will attempt to access non-existent methods on None. But not sure there is any way around this, at least it is an error rather than strange behaviour. But definitely makes sense to do this now further to discussions on API stability #2303 and release of SortingAnalyzer.

I don't think it can hurt to add a little more detail on the assert message. Possibly including that the behaviour of set_probe was changed in version XXXX (at least, this can be included temporarily, just so people don't think they are going crazy) and explicitly state they should call with set_inplace=False.

[zm711]

View #3777 where this confusion has appeared again. I personally prefer the idea of doing nothing in place, but that would likely require a different user facing function not loaded with the ideas of set. Just to keep this issue in our mind.

[h-mayorquin]

The side that wants the non-setting behavior should just come up with a name of your choice guys. We can then move forward.

[JoeZiminski]

I like create_new_recording_with_probe or get_recording_with_probe, I think I prefer its on the recording object than BaseRecording, even though BaseRecording makes more sense its inconvenient to find / remember the import path and confusing for newer users or those not familiar with OOP.