Move towards making set_probe in place by default

[h-mayorquin]

Should fix https://github.com/SpikeInterface/spikeinterface/issues/2193

[JoeZiminski]

Personally I don't mind if it returns a recording (for API consistency) or sets in place. But if it returns a recording I think it should be renamed from set_probe because that implies (to me at least) an in-place operation.

For probe vs. probegroup I would agree it makes sense to reflect the general existing API here too. If wanting to change in future, it is easier to take a single-decision to merge these concepts at a higher level and change all instances at once.

[zm711]

set_probe because that implies (to me at least) an in-place operation.

Joe's comment actually reminded me the point of set so I've changed my mind. set should be in_place so I retract my previous statement.

For probe vs. probegroup I would agree it makes sense to reflect the general existing API here too. If wanting to change in future, it is easier to take a single-decision to merge these concepts at a higher level and change all instances at once.

Yep exactly!

[h-mayorquin]

As mentioned above, there are two functions set_probe and set_probes, it seems that set_probes is more general. I suggest opening another issue if we want to discuss the probe setting API. The aim of this is just to change the functions to be in place.

Am I missing something?

[zm711]

I was referring to the new function you made called create_with_probes, which is part of this api. But let me see if I can find the function set_probes. My point was actually that I would prefer

create_with_probe create_with_probegroup

to mimic the functions I normally use set_probe and set_probegroup. I have never seen the set_probes function.

[zm711]

See line 99 which internally uses set_probes. I assumed maybe wrongly that set_probe and set_probegroup the only public "right way" to set a probe vs a probegroup.

[h-mayorquin]

All the functions are public so I think you would have to ask @samuelgarcia and @alejoe91 what is the "right way" (intendend) that they envisioned or want. Usually they prefer general functions that handle a bunch of cases (see that they prefer to use recording.save() vs recording.save_to_* () and in general I see them adding keyword to functions instead of creating a function for a different behavior) so I did in this way. I agree with you that the creation functions should mimick the setting functions once we decide on the right way.

[zm711]

Agreed. I don't think I've ever seen us document set_probes which is what I meant by "right way", but as you said it is a public function so people could use. Happy to wait to see what they say.

[h-mayorquin]

Agreed. I don't think I've ever seen us document set_probes which is what I meant by "right way", but as you said it is a public function so people could use. Happy to wait to see what they say.

Looking the code base it seems that set_probe and set_probes are more commonly used on the tests. I only see one use case of set_probes.

[zm711]

I just searched the docs and set_probe is mentioned like 5 times. set_probegroup is mentioned once and set_probes is never mentioned (again based on the search function).

[h-mayorquin]

Which search function are you using?

I searched before the claim above. The one use case is here and by someone that is not "us":

https://github.com/h-mayorquin/spikeinterface/blob/4f4fafb96382676c63949647b172ee013f35a290/src/spikeinterface/extractors/neoextractors/spikegadgets.py#L43-L44

[zm711]

The search function on readthedocs. I'm talking about rtd documentation.

[h-mayorquin]

Got you. I think the evidence points in the direction of your claim, though. I will make these two methods. I think we probably should make the most general method set_probes private but that's probably another issue / PR.

[alejoe91]

Another issue here: https://github.com/SpikeInterface/spikeinterface/issues/2895