Repository: catalystneuro/neuroconv

Issue:
Exclude physical_unit properties from electrodes table
Recently (I'm not sure when) SpikeInterface added the following properties to their recording extractors: `gain_to_physical_unit`, `offset_to_physical_unit`, and `physical_unit`. I believe these properties should be added to the list of excluded properties in `add_electrodes_to_nwbfile` [here](https://github.com/catalystneuro/neuroconv/blob/5f609e2fcde8a62388edf70a4433508ebb6e99e6/src/neuroconv/tools/spikeinterface/spikeinterface.py#L824), since they are recorded in the ElectricalSeries object instead of the electrodes table.

Note, I'd like to try to solve this issue with the copilot agent, which is why I've added the copilot label. 

Discussion hints:
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/catalystneuro/neuroconv/pull/1406?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=catalystneuro) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 89.64%. Comparing base [(`ae2fb24`)](https://app.codecov.io/gh/catalystneuro/neuroconv/commit/ae2fb24432bc4e15e660e287128f0f26dbba01f9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=catalystneuro) to head [(`d068395`)](https://app.codecov.io/gh/catalystneuro/neuroconv/commit/d068395cb6e3ad72d8ebbcba8a330336d18220bd?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=catalystneuro).
> Report is 2 commits behind head on main.

<details><summary>Additional details and impacted files</summary>


[![Impacted file tree graph](https://app.codecov.io/gh/catalystneuro/neuroconv/pull/1406/graphs/tree.svg?width=650&height=150&src=pr&token=h63OQoWehP&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=catalystneuro)](https://app.codecov.io/gh/catalystneuro/neuroconv/pull/1406?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=catalystneuro)

```diff
@@            Coverage Diff             @@
##             main    #1406      +/-   ##
==========================================
- Coverage   90.06%   89.64%   -0.42%     
==========================================
  Files         141      144       +3     
  Lines        9126     9362     +236     
==========================================
+ Hits         8219     8393     +174     
- Misses        907      969      +62     
```

| [Flag](https://app.codecov.io/gh/catalystneuro/neuroconv/pull/1406/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=catalystneuro) | Coverage Δ | |
|---|---|---|
| [unittests](https://app.codecov.io/gh/catalystneuro/neuroconv/pull/1406/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=catalystneuro) | `89.64% <ø> (-0.42%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=catalystneuro#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Files with missing lines](https://app.codecov.io/gh/catalystneuro/neuroconv/pull/1406?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=catalystneuro) | Coverage Δ | |
|---|---|---|
| [...c/neuroconv/tools/spikeinterface/spikeinterface.py](https://app.codecov.io/gh/catalystneuro/neuroconv/pull/1406?src=pr&el=tree&filepath=src%2Fneuroconv%2Ftools%2Fspikeinterface%2Fspikeinterface.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=catalystneuro#diff-c3JjL25ldXJvY29udi90b29scy9zcGlrZWludGVyZmFjZS9zcGlrZWludGVyZmFjZS5weQ==) | `92.73% <ø> (ø)` | |

</details>
<details><summary> :rocket: New features to boost your workflow: </summary>

- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>

Solution patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index f2a924b4..7f78611b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,7 @@
 
 
 ## Bug Fixes
+* Fixed SpikeInterface physical unit properties being incorrectly included in electrodes table [PR #1406](https://github.com/catalystneuro/neuroconv/pull/1406)
 
 ## Features
 * Segmentation interfaces now support roi ids that are strings [PR #1390](https://github.com/catalystneuro/neuroconv/pull/1390)
diff --git a/src/neuroconv/tools/spikeinterface/spikeinterface.py b/src/neuroconv/tools/spikeinterface/spikeinterface.py
index 80e0c76f..2bde330f 100644
--- a/src/neuroconv/tools/spikeinterface/spikeinterface.py
+++ b/src/neuroconv/tools/spikeinterface/spikeinterface.py
@@ -811,6 +811,9 @@ def add_electrodes_to_nwbfile(
     spikeinterface_special_cases = [
         "offset_to_uV",  # Written in the ElectricalSeries
         "gain_to_uV",  # Written in the ElectricalSeries
+        "gain_to_physical_unit",  # Written in the ElectricalSeries
+        "offset_to_physical_unit",  # Written in the ElectricalSeries
+        "physical_unit",  # Written in the ElectricalSeries
         "contact_vector",  # Structured array representing the probe
         "channel_name",  # We handle this here with _get_channel_name
         "channel_names",  # Some formats from neo also have this property, skip it


Test patch:
diff --git a/tests/test_modalities/test_ecephys/test_tools_spikeinterface.py b/tests/test_modalities/test_ecephys/test_tools_spikeinterface.py
index 34bfc64c..aa8acb2f 100644
--- a/tests/test_modalities/test_ecephys/test_tools_spikeinterface.py
+++ b/tests/test_modalities/test_ecephys/test_tools_spikeinterface.py
@@ -585,6 +585,25 @@ class TestAddElectrodes(TestCase):
         actual_electrode_column_names = list(self.nwbfile.electrodes.colnames)
         self.assertCountEqual(actual_electrode_column_names, expected_electrode_column_names)
 
+    def test_physical_unit_properties_excluded(self):
+        """Test that SpikeInterface physical unit properties are excluded from electrodes table."""
+        # Set the properties that should be excluded
+        num_channels = len(self.base_recording.channel_ids)
+        self.base_recording.set_property(key="gain_to_physical_unit", values=[1.0] * num_channels)
+        self.base_recording.set_property(key="offset_to_physical_unit", values=[0.0] * num_channels)
+        self.base_recording.set_property(key="physical_unit", values=["uV"] * num_channels)
+
+        add_electrodes_to_nwbfile(recording=self.base_recording, nwbfile=self.nwbfile)
+
+        # Verify that these properties are NOT in the electrodes table
+        actual_electrode_column_names = list(self.nwbfile.electrodes.colnames)
+        excluded_properties = ["gain_to_physical_unit", "offset_to_physical_unit", "physical_unit"]
+
+        for prop in excluded_properties:
+            self.assertNotIn(
+                prop, actual_electrode_column_names, f"Property '{prop}' should be excluded from electrodes table"
+            )
+
     def test_integer_channel_names(self):
         """Ensure channel names merge correctly after appending when channel names are integers."""
         channel_ids = self.base_recording.get_channel_ids()