From 86b820edc4ec4472a7417581de2797786c4f9781 Mon Sep 17 00:00:00 2001 From: Heberto Mayorquin Date: Wed, 18 Dec 2024 16:30:27 -0600 Subject: [PATCH 1/6] add failing test --- src/neuroconv/tools/testing/mock_interfaces.py | 6 ++++++ tests/test_ecephys/test_ecephys_interfaces.py | 14 +++++++++++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/neuroconv/tools/testing/mock_interfaces.py b/src/neuroconv/tools/testing/mock_interfaces.py index 38cc750ab..867ec12c4 100644 --- a/src/neuroconv/tools/testing/mock_interfaces.py +++ b/src/neuroconv/tools/testing/mock_interfaces.py @@ -220,6 +220,12 @@ def __init__( es_key=es_key, ) + # Adding this as a safeguard before the spikeinterface changes are merged: + # https://github.com/SpikeInterface/spikeinterface/pull/3588 + channel_ids = self.recording_extractor.get_channel_ids() + channel_ids_as_strings = [str(id) for id in channel_ids] + self.recording_extractor = self.recording_extractor.rename_channels(new_channel_ids=channel_ids_as_strings) + def get_metadata(self) -> dict: """ Returns the metadata dictionary for the current object. diff --git a/tests/test_ecephys/test_ecephys_interfaces.py b/tests/test_ecephys/test_ecephys_interfaces.py index 168ce5068..d2fdd68ba 100644 --- a/tests/test_ecephys/test_ecephys_interfaces.py +++ b/tests/test_ecephys/test_ecephys_interfaces.py @@ -119,7 +119,7 @@ def test_electrode_indices_assertion_error_when_missing_table(self, setup_interf class TestRecordingInterface(RecordingExtractorInterfaceTestMixin): data_interface_cls = MockRecordingInterface - interface_kwargs = dict(durations=[0.100]) + interface_kwargs = dict(num_channels=4, durations=[0.100]) def test_stub(self, setup_interface): interface = self.interface @@ -146,6 +146,18 @@ def test_always_write_timestamps(self, setup_interface): expected_timestamps = self.interface.recording_extractor.get_times() np.testing.assert_array_equal(electrical_series.timestamps[:], expected_timestamps) + def test_group_naming_not_adding_extra_devices(self, setup_interface): + + interface = self.interface + recording_extractor = interface.recording_extractor + recording_extractor.set_channel_groups(groups=[0, 1, 2, 3]) + recording_extractor.set_property(key="group_name", values=["group1", "group2", "group3", "group4"]) + + nwbfile = interface.create_nwbfile() + + assert len(nwbfile.devices) == 1 + assert len(nwbfile.electrode_groups) == 4 + class TestAssertions(TestCase): @pytest.mark.skipif(python_version.minor != 10, reason="Only testing with Python 3.10!") From 333ad6963713d7f54d7b6c72f8f7312c24cbead3 Mon Sep 17 00:00:00 2001 From: Heberto Mayorquin Date: Wed, 18 Dec 2024 16:34:46 -0600 Subject: [PATCH 2/6] add passing test --- .../datainterfaces/ecephys/baserecordingextractorinterface.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/neuroconv/datainterfaces/ecephys/baserecordingextractorinterface.py b/src/neuroconv/datainterfaces/ecephys/baserecordingextractorinterface.py index 6d0df14c1..c9df3ba52 100644 --- a/src/neuroconv/datainterfaces/ecephys/baserecordingextractorinterface.py +++ b/src/neuroconv/datainterfaces/ecephys/baserecordingextractorinterface.py @@ -88,7 +88,9 @@ def get_metadata_schema(self) -> dict: def get_metadata(self) -> DeepDict: metadata = super().get_metadata() - channel_groups_array = self.recording_extractor.get_channel_groups() + from ...tools.spikeinterface.spikeinterface import _get_group_name + + channel_groups_array = _get_group_name(recording=self.recording_extractor) unique_channel_groups = set(channel_groups_array) if channel_groups_array is not None else ["ElectrodeGroup"] electrode_metadata = [ dict(name=str(group_id), description="no description", location="unknown", device="DeviceEcephys") From c8f91c7e795edc2b92d0de2376db8211fe88207c Mon Sep 17 00:00:00 2001 From: Heberto Mayorquin Date: Wed, 18 Dec 2024 16:39:00 -0600 Subject: [PATCH 3/6] add changelog --- CHANGELOG.md | 2 ++ src/neuroconv/tools/testing/mock_interfaces.py | 6 ------ 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 68f5c9868..7488d5834 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,8 @@ * Fix a bug where data in `DeepLabCutInterface` failed to write when `ndx-pose` was not imported. [#1144](https://github.com/catalystneuro/neuroconv/pull/1144) * `SpikeGLXConverterPipe` converter now accepts multi-probe structures with multi-trigger and does not assume a specific folder structure [#1150](https://github.com/catalystneuro/neuroconv/pull/1150) * `SpikeGLXNIDQInterface` is no longer written as an ElectricalSeries [#1152](https://github.com/catalystneuro/neuroconv/pull/1152) +* Fix a bug on ecephys interfaces where extra electrode group and devices were written if the property of the "group_name" was set in the recording extractor [#1164](https://github.com/catalystneuro/neuroconv/pull/1164) + ## Features * Propagate the `unit_electrode_indices` argument from the spikeinterface tools to `BaseSortingExtractorInterface`. This allows users to map units to the electrode table when adding sorting data [PR #1124](https://github.com/catalystneuro/neuroconv/pull/1124) diff --git a/src/neuroconv/tools/testing/mock_interfaces.py b/src/neuroconv/tools/testing/mock_interfaces.py index 867ec12c4..38cc750ab 100644 --- a/src/neuroconv/tools/testing/mock_interfaces.py +++ b/src/neuroconv/tools/testing/mock_interfaces.py @@ -220,12 +220,6 @@ def __init__( es_key=es_key, ) - # Adding this as a safeguard before the spikeinterface changes are merged: - # https://github.com/SpikeInterface/spikeinterface/pull/3588 - channel_ids = self.recording_extractor.get_channel_ids() - channel_ids_as_strings = [str(id) for id in channel_ids] - self.recording_extractor = self.recording_extractor.rename_channels(new_channel_ids=channel_ids_as_strings) - def get_metadata(self) -> dict: """ Returns the metadata dictionary for the current object. From 3a8a8b15ff68049a9d833e2559cc2adb0296a8f7 Mon Sep 17 00:00:00 2001 From: Heberto Mayorquin Date: Wed, 18 Dec 2024 16:55:07 -0600 Subject: [PATCH 4/6] add tests to detect mismatch errors and fix it --- .../tools/spikeinterface/spikeinterface.py | 27 ++++++++++++++++++- .../tools/testing/mock_interfaces.py | 7 +++++ .../test_ecephys/test_tools_spikeinterface.py | 24 +++++++++++++++++ 3 files changed, 57 insertions(+), 1 deletion(-) diff --git a/src/neuroconv/tools/spikeinterface/spikeinterface.py b/src/neuroconv/tools/spikeinterface/spikeinterface.py index d31e6032c..01232ad22 100644 --- a/src/neuroconv/tools/spikeinterface/spikeinterface.py +++ b/src/neuroconv/tools/spikeinterface/spikeinterface.py @@ -245,11 +245,19 @@ def _get_group_name(recording: BaseRecording) -> np.ndarray: An array containing the group names. If the `group_name` property is not available, the channel groups will be returned. If the group names are empty, a default value 'ElectrodeGroup' will be used. + + Raises + ------ + ValueError + If the number of unique group names doesn't match the number of unique groups, + or if the mapping between group names and group numbers is inconsistent. """ default_value = "ElectrodeGroup" group_names = recording.get_property("group_name") + groups = recording.get_channel_groups() + if group_names is None: - group_names = recording.get_channel_groups() + group_names = groups if group_names is None: group_names = np.full(recording.get_num_channels(), fill_value=default_value) @@ -259,6 +267,23 @@ def _get_group_name(recording: BaseRecording) -> np.ndarray: # If for any reason the group names are empty, fill them with the default group_names[group_names == ""] = default_value + # Validate group names against groups + if groups is not None: + unique_groups = np.unique(groups) + unique_names = np.unique(group_names) + + if len(unique_names) != len(unique_groups): + raise ValueError("The number of group names must match the number of groups") + + # Check consistency of group name to group number mapping + group_to_name_map = {} + for group, name in zip(groups, group_names): + if group in group_to_name_map: + if group_to_name_map[group] != name: + raise ValueError("Inconsistent mapping between group numbers and group names") + else: + group_to_name_map[group] = name + return group_names diff --git a/src/neuroconv/tools/testing/mock_interfaces.py b/src/neuroconv/tools/testing/mock_interfaces.py index 38cc750ab..42bf699df 100644 --- a/src/neuroconv/tools/testing/mock_interfaces.py +++ b/src/neuroconv/tools/testing/mock_interfaces.py @@ -220,6 +220,12 @@ def __init__( es_key=es_key, ) + # Adding this as a safeguard before the spikeinterface changes are merged: + # https://github.com/SpikeInterface/spikeinterface/pull/3588 + channel_ids = self.recording_extractor.get_channel_ids() + channel_ids_as_strings = [str(id) for id in channel_ids] + self.recording_extractor = self.recording_extractor.rename_channels(new_channel_ids=channel_ids_as_strings) + def get_metadata(self) -> dict: """ Returns the metadata dictionary for the current object. @@ -272,6 +278,7 @@ def __init__( ) # Sorting extractor to have string unit ids until is changed in SpikeInterface + # https://github.com/SpikeInterface/spikeinterface/pull/3588 string_unit_ids = [str(id) for id in self.sorting_extractor.unit_ids] self.sorting_extractor = self.sorting_extractor.rename_units(new_unit_ids=string_unit_ids) diff --git a/tests/test_ecephys/test_tools_spikeinterface.py b/tests/test_ecephys/test_tools_spikeinterface.py index 3436a2e70..68c681f46 100644 --- a/tests/test_ecephys/test_tools_spikeinterface.py +++ b/tests/test_ecephys/test_tools_spikeinterface.py @@ -24,6 +24,7 @@ from neuroconv.tools.nwb_helpers import get_module from neuroconv.tools.spikeinterface import ( add_electrical_series_to_nwbfile, + add_electrode_groups_to_nwbfile, add_electrodes_to_nwbfile, add_recording_to_nwbfile, add_sorting_to_nwbfile, @@ -1071,6 +1072,29 @@ def test_missing_bool_values(self): assert np.array_equal(extracted_incomplete_property, expected_incomplete_property) +class TestAddElectrodeGroups: + def test_group_naming_not_matching_group_number(self): + recording = generate_recording(num_channels=4) + recording.set_channel_groups(groups=[0, 1, 2, 3]) + recording.set_property(key="group_name", values=["A", "A", "A", "A"]) + + nwbfile = mock_NWBFile() + with pytest.raises(ValueError, match="The number of group names must match the number of groups"): + add_electrode_groups_to_nwbfile(nwbfile=nwbfile, recording=recording) + + def test_inconsistent_group_name_mapping(self): + recording = generate_recording(num_channels=3) + # Set up groups where the same group name is used for different group numbers + recording.set_channel_groups(groups=[0, 1, 0]) + recording.set_property( + key="group_name", values=["A", "B", "B"] # Inconsistent: group 0 maps to names "A" and "B" + ) + + nwbfile = mock_NWBFile() + with pytest.raises(ValueError, match="Inconsistent mapping between group numbers and group names"): + add_electrode_groups_to_nwbfile(nwbfile=nwbfile, recording=recording) + + class TestAddUnitsTable(TestCase): @classmethod def setUpClass(cls): From cf579d6946649bc026aefe4ab2febd24f00d6602 Mon Sep 17 00:00:00 2001 From: Heberto Mayorquin Date: Wed, 18 Dec 2024 17:02:57 -0600 Subject: [PATCH 5/6] CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7488d5834..e40382bf3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,7 @@ * Use pytest format for dandi tests to avoid window permission error on teardown [PR #1151](https://github.com/catalystneuro/neuroconv/pull/1151) * Added many docstrings for public functions [PR #1063](https://github.com/catalystneuro/neuroconv/pull/1063) * Clean up with warnings and deprecations in the testing framework [PR #1158](https://github.com/catalystneuro/neuroconv/pull/1158) +* Detect mismatch errors between group and group names when writing ElectrodeGroups [PR #1165](https://github.com/catalystneuro/neuroconv/pull/1165) # v0.6.5 (November 1, 2024) From 366bc8faf89a30d6830333419f6f2d5124c08ebf Mon Sep 17 00:00:00 2001 From: Heberto Mayorquin Date: Thu, 19 Dec 2024 10:09:29 -0600 Subject: [PATCH 6/6] use set instead of np.unique --- src/neuroconv/tools/spikeinterface/spikeinterface.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/neuroconv/tools/spikeinterface/spikeinterface.py b/src/neuroconv/tools/spikeinterface/spikeinterface.py index 01232ad22..7df25c703 100644 --- a/src/neuroconv/tools/spikeinterface/spikeinterface.py +++ b/src/neuroconv/tools/spikeinterface/spikeinterface.py @@ -269,8 +269,8 @@ def _get_group_name(recording: BaseRecording) -> np.ndarray: # Validate group names against groups if groups is not None: - unique_groups = np.unique(groups) - unique_names = np.unique(group_names) + unique_groups = set(groups) + unique_names = set(group_names) if len(unique_names) != len(unique_groups): raise ValueError("The number of group names must match the number of groups")