Skip to content

Move towards making set_probe in place by default #2798

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Move towards making set_probe in place by default #2798

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba9417a
move towards making set_probe in place
h-mayorquin May 3, 2024
5f2d227
fix warning
h-mayorquin May 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 33 additions & 3 deletions src/spikeinterface/core/baserecordingsnippets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@
from pathlib import Path

import numpy as np
import warnings

from probeinterface import Probe, ProbeGroup, write_probeinterface, read_probeinterface, select_axes

from .base import BaseExtractor
from .core_tools import check_json
from .recording_tools import check_probe_do_not_overlap

from warnings import warn
Expand Down Expand Up @@ -76,7 +76,7 @@ def set_probe(self, probe, group_mode="by_probe", in_place=False):

Parameters
----------
probe_or_probegroup: Probe, list of Probe, or ProbeGroup
probe: a Probe object
The probe(s) to be attached to the recording
group_mode: "by_probe" | "by_shank", default: "by_probe
"by_probe" or "by_shank". Adds grouping property to the recording based on the probes ("by_probe")
Expand All @@ -90,6 +90,7 @@ def set_probe(self, probe, group_mode="by_probe", in_place=False):
sub_recording: BaseRecording
A view of the recording (ChannelSlice or clone or itself)
"""

assert isinstance(probe, Probe), "must give Probe"
probegroup = ProbeGroup()
probegroup.add_probe(probe)
Expand Down Expand Up @@ -173,7 +174,15 @@ def set_probes(self, probe_or_probegroup, group_mode="by_probe", in_place=False)
probe_as_numpy_array = probe_as_numpy_array[order]
probe_as_numpy_array["device_channel_indices"] = np.arange(probe_as_numpy_array.size, dtype="int64")

# create recording : channel slice or clone or self
deprecation_msg = (
"in_place will change its default to True in version 0.103 and stop returning a recording"
" use recording.create_with_probes(probe_or_probegroup) instead to replicate the old functionality"
)
warnings.warn(
deprecation_msg,
DeprecationWarning,
stacklevel=2,
)
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")
Expand Down Expand Up @@ -225,6 +234,27 @@ def set_probes(self, probe_or_probegroup, group_mode="by_probe", in_place=False)

return sub_recording

def create_with_probes(self, probe_or_probegroup, group_mode="by_probe"):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it should be create_with_probe vs probes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"""
Creates a new recording with a probe attached.

Parameters
----------
probe_or_probegroup: Probe, list of Probes, or ProbeGroup
The probe(s) to be attached to the recording
group_mode: "by_probe" | "by_shank", default: "by_probe"
"by_probe" or "by_shank". Adds grouping property to the recording based on the probes ("by_probe")
or shanks ("by_shank")

Returns
-------
new_recording: BaseRecording
A new recording object with the probe attached
"""

# TODO: Once in_place is True by default in set_probes, we might re-organize the code
return self.set_probegroup(probe_or_probegroup, in_place=False)

def get_probe(self):
probes = self.get_probes()
assert len(probes) == 1, "there are several probe use .get_probes() or get_probegroup()"
Expand Down