Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

set_probe behavior for RecordingExtractors #2193

Open
h-mayorquin opened this issue Nov 12, 2023 · 4 comments · May be fixed by #2798
Open

set_probe behavior for RecordingExtractors #2193

h-mayorquin opened this issue Nov 12, 2023 · 4 comments · May be fixed by #2798
Labels
core Changes to core module discussion General discussions and community feedback probeinterface Related to probeinterface

Comments

@h-mayorquin
Copy link
Collaborator

h-mayorquin commented Nov 12, 2023 •
edited

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.
@h-mayorquin h-mayorquin added core Changes to core module probeinterface Related to probeinterface discussion General discussions and community feedback labels Nov 12, 2023
@alejoe91
Copy link
Member

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
Copy link
Member

magland commented Nov 13, 2023

Thanks @h-mayorquin

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

BaseRecording.create_with_probe(recording, probe=probe)

@magland
Copy link
Member

magland commented Nov 13, 2023

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.

@h-mayorquin h-mayorquin mentioned this issue Mar 15, 2024
Merged
This was referenced May 3, 2024
Merged
Draft
@JoeZiminski
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
core Changes to core module discussion General discussions and community feedback probeinterface Related to probeinterface
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Move towards making set_probe in place by default h-mayorquin/spikeinterface
4 participants
@magland @h-mayorquin @alejoe91 @JoeZiminski