Move towards making set_probe in place by default

[h-mayorquin]

Should fix #2193

[zm711]

For me I liked the always return (i.e. in_place always False), but from the debate it looks like it went the other direction. (That's what I get for being silent).

So my only comment would more be discoverability/understandability. We distinguish between set_probe and set_probegroup at the user-level, but now this function does both. Why? shouldn't we have a create_with_probe and a separate create_with_probegroup to maintain the previous api for users. We don't really have the concept of probes right? Instead you choose between A probe and A probegroup neither being plural for the api.

EDIT: just to be clear I do support making this a one option function in general. The rest of the api always returns a new object so having this function be the one function that gives users the choice is super confusing. For me I was leaning toward return just for api consistency but otherwise I don't care which way it works.

[zm711]

I wonder if it should be create_with_probe vs probes.

[h-mayorquin]

Ther are two functions, the one with the plural name is the most general as it takes both probe a list or a probe group, so that's where I want the the warnin to be. I wish there was only one function and if you see the diff in the changes it seems that that was the intention of one point.

Simplifying probe setting could be a issue of its own, I am not making any changes in that direction in this PR. It is just to get the ball rolling to make the behaivor of the function consistent with setter semantics.

[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: #2895