Remove most required arguments from electrodes table (#1448)

Co-authored-by: Cody Baker <[email protected]> Co-authored-by: Ryan Ly <[email protected]> Co-authored-by: Ben Dichter <[email protected]>
NeurodataWithoutBorders · Jun 7, 2022 · cf0ee6d · cf0ee6d
1 parent d38cc78
commit cf0ee6d
Show file tree

Hide file tree

Showing 9 changed files with 102 additions and 58 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,7 +15,10 @@
   change, the impact user codes should be minimal as this change primarily adds functionality while the overall
   behavior of the API is largely consistent with existing behavior. @oruebel, @rly (#1390)
 
-# Enhancements
+# Enhancements and minor changes
+- The arguments x, y, z, imp, location, filtering are no longer required in the electrodes table.
+- Added `cell_id` attribute to `IntracellularElectrode`. @bendichter (#1459)
+- Added `offset` field to `TimeSeries` and its subtypes. @codycbakerphd (#1424)
 - Added support for NWB 2.5.0.
   - Added support for updated ``IndexSeries`` type, new ``order_of_images`` field in ``Images``, and new neurodata_type
     ``ImageReferences``. @rly (#1483)
@@ -27,16 +30,23 @@
 - Added tutorial on annotating data via ``TimeIntervals``. @oruebel (#1390)
 - Add copy button to code blocks @weiglszonja (#1460)
 - Create behavioral tutorial @weiglszonja (#1464)
-- Enhance display of icephys pandas tutorial by using ``dataframe_image`` to render and display large tables as images. @oruebel (#1469)
+- Enhance display of icephys pandas tutorial by using ``dataframe_image`` to render and display large tables
+  as images. @oruebel (#1469)
 - Create tutorial about reading and exploring an existing `NWBFile` @weiglszonja (#1453)
+- Added new logo for PyNWB. @oruebel (#1461)
+- Minor text fixes. @oruebel @bendichter (#1443, #1462, #1463, #1466, #1472, #1473)
+
+### Bug fixes:
+- Fixed input data types to allow only `float` for fields `conversion` and `offset` in definition of
+  ``TimeSeries``. @codycbakerphd (#1424)
+
 
 ## PyNWB 2.0.1 (March 16, 2022)
 
 ### Bug fixes:
 - Add `environment-ros3.yml` to `MANIFEST.in` for inclusion in source distributions. @rly (#1398)
 - Fix bad error check in ``IntracellularRecordingsTable.add_recording`` when adding ``IZeroClampSeries``. @oruebel (#1410)
 - Skip ros3 tests if internet access or the ros3 driver are not available. @oruebel (#1414)
--  Fixed input data types to allow only `float` for fields `conversion` and `offset` in definition of ``TimeSeries``. @codycbakerphd (#1424)
 - Fix CI issues. @rly (#1427)
 
 ### Documentation and tutorial enhancements:
@@ -56,7 +66,6 @@
 ### Minor improvements:
 - Improve constructor docstrings for Image types. @weiglszonja (#1418)
 - Add checks for data orientation in ``TimeSeries``, ``ElectricalSeries``, and ``RoiResponseSeries`` @bendichter (#1428)
-- Added `offset` field to `TimeSeries` and its subtypes. @codycbakerphd (#1424)
 - Added checks for data orientation in ``TimeSeries``, ``ElectricalSeries``, and ``RoiResponseSeries``.
   @bendichter (#1426)
 - Enhanced issue template forms on GitHub. @CodyCBakerPHD (#1434)

diff --git a/src/pynwb/file.py b/src/pynwb/file.py
@@ -7,7 +7,7 @@
 import numpy as np
 import pandas as pd
 
-from hdmf.utils import docval, getargs, get_docval, popargs, popargs_to_dict
+from hdmf.utils import docval, getargs, get_docval, popargs, popargs_to_dict, AllowPositional
 
 from . import register_class, CORE_NAMESPACE
 from .base import TimeSeries, ProcessingModule
@@ -605,41 +605,67 @@ def add_electrode_column(self, **kwargs):
         self.__check_electrodes()
         self.electrodes.add_column(**kwargs)
 
-    @docval({'name': 'x', 'type': 'float', 'doc': 'the x coordinate of the position (+x is posterior)'},
-            {'name': 'y', 'type': 'float', 'doc': 'the y coordinate of the position (+y is inferior)'},
-            {'name': 'z', 'type': 'float', 'doc': 'the z coordinate of the position (+z is right)'},
-            {'name': 'imp', 'type': 'float', 'doc': 'the impedance of the electrode, in ohms'},
-            {'name': 'location', 'type': str, 'doc': 'the location of electrode within the subject e.g. brain region'},
+    @docval({'name': 'x', 'type': 'float', 'doc': 'the x coordinate of the position (+x is posterior)',
+             'default': None},
+            {'name': 'y', 'type': 'float', 'doc': 'the y coordinate of the position (+y is inferior)', 'default': None},
+            {'name': 'z', 'type': 'float', 'doc': 'the z coordinate of the position (+z is right)', 'default': None},
+            {'name': 'imp', 'type': 'float', 'doc': 'the impedance of the electrode, in ohms', 'default': None},
+            {'name': 'location', 'type': str,
+             'doc': 'the location of electrode within the subject e.g. brain region. Required.',
+             'default': None},
             {'name': 'filtering', 'type': str,
-             'doc': 'description of hardware filtering, including the filter name and frequency cutoffs'},
-            {'name': 'group', 'type': ElectrodeGroup, 'doc': 'the ElectrodeGroup object to add to this NWBFile'},
+             'doc': 'description of hardware filtering, including the filter name and frequency cutoffs',
+             'default': None},
+            {'name': 'group', 'type': ElectrodeGroup,
+             'doc': 'the ElectrodeGroup object to add to this NWBFile. Required.',
+             'default': None},
             {'name': 'id', 'type': int, 'doc': 'a unique identifier for the electrode', 'default': None},
             {'name': 'rel_x', 'type': 'float', 'doc': 'the x coordinate within the electrode group', 'default': None},
             {'name': 'rel_y', 'type': 'float', 'doc': 'the y coordinate within the electrode group', 'default': None},
             {'name': 'rel_z', 'type': 'float', 'doc': 'the z coordinate within the electrode group', 'default': None},
-            {'name': 'reference', 'type': str, 'doc': 'Description of the reference used for this electrode.',
-             'default': None},
+            {'name': 'reference', 'type': str, 'doc': 'Description of the reference electrode and/or reference scheme\
+                used for this  electrode, e.g.,"stainless steel skull screw" or "online common average referencing". ',
+                'default': None},
             {'name': 'enforce_unique_id', 'type': bool, 'doc': 'enforce that the id in the table must be unique',
              'default': True},
-            allow_extra=True)
+            allow_extra=True,
+            allow_positional=AllowPositional.WARNING)
     def add_electrode(self, **kwargs):
         """
         Add an electrode to the electrodes table.
         See :py:meth:`~hdmf.common.DynamicTable.add_row` for more details.
 
-        Required fields are *x*, *y*, *z*, *imp*, *location*, *filtering*,
+        Required fields are *location* and
         *group* and any columns that have been added
         (through calls to `add_electrode_columns`).
         """
         self.__check_electrodes()
         d = _copy.copy(kwargs['data']) if kwargs.get('data') is not None else kwargs
+
+        # NOTE location and group are required arguments. in PyNWB 2.1.0 we made x, y, z optional arguments, and
+        # in order to avoid breaking API changes, the order of the arguments needed to be maintained even though
+        # these optional arguments came before the required arguments, so in docval these required arguments are
+        # displayed as optional when really they are required. this should be changed when positional arguments
+        # are not allowed
+        if not d['location']:
+            raise ValueError("The 'location' argument is required when creating an electrode.")
+        if not kwargs['group']:
+            raise ValueError("The 'group' argument is required when creating an electrode.")
         if d.get('group_name', None) is None:
             d['group_name'] = d['group'].name
 
-        new_cols = [('rel_x', 'the x coordinate within the electrode group'),
+        new_cols = [('x', 'the x coordinate of the position (+x is posterior)'),
+                    ('y', 'the y coordinate of the position (+y is inferior)'),
+                    ('z', 'the z coordinate of the position (+z is right)'),
+                    ('imp', 'the impedance of the electrode, in ohms'),
+                    ('filtering', 'description of hardware filtering, including the filter name and frequency cutoffs'),
+                    ('rel_x', 'the x coordinate within the electrode group'),
                     ('rel_y', 'the y coordinate within the electrode group'),
                     ('rel_z', 'the z coordinate within the electrode group'),
-                    ('reference', 'Description of the reference used for this electrode.')]
+                    ('reference', 'Description of the reference electrode and/or reference scheme used for this \
+                        electrode, e.g.,"stainless steel skull screw" or "online common average referencing".')
+                    ]
+
         # add column if the arg is supplied and column does not yet exist
         # do not pass arg to add_row if arg is not supplied
         for col_name, col_doc in new_cols:
@@ -1091,12 +1117,7 @@ def _tablefunc(table_name, description, columns):
 def ElectrodeTable(name='electrodes',
                    description='metadata about extracellular electrodes'):
     return _tablefunc(name, description,
-                      [('x', 'the x coordinate of the channel location'),
-                       ('y', 'the y coordinate of the channel location'),
-                       ('z', 'the z coordinate of the channel location'),
-                       ('imp', 'the impedance of the channel'),
-                       ('location', 'the location of channel within the subject e.g. brain region'),
-                       ('filtering', 'description of hardware filtering'),
+                      [('location', 'the location of channel within the subject e.g. brain region'),
                        ('group', 'a reference to the ElectrodeGroup this electrode is a part of'),
                        ('group_name', 'the name of the ElectrodeGroup this electrode is a part of')
                        ]

diff --git a/src/pynwb/nwb-schema b/src/pynwb/nwb-schema
diff --git a/tests/integration/hdf5/test_ecephys.py b/tests/integration/hdf5/test_ecephys.py
@@ -40,8 +40,7 @@ def make_electrode_table(self):
                                     location='tetrode location',
                                     device=self.dev1)
         for i in range(4):
-            self.table.add_row(x=float(i), y=2.0, z=3.0, imp=-1.0, location='CA1', filtering='none', group=self.group,
-                               group_name='tetrode1')
+            self.table.add_row(location='CA1', group=self.group, group_name='tetrode1')
 
     def setUpContainer(self):
         """ Return the test ElectricalSeries to read/write """

diff --git a/tests/integration/hdf5/test_io.py b/tests/integration/hdf5/test_io.py
@@ -247,7 +247,7 @@ def test_append(self):
             location='',
             device=device
         )
-        self.nwbfile.add_electrode(x=0.0, y=0.0, z=0.0, imp=np.nan, location='', filtering='', group=e_group)
+        self.nwbfile.add_electrode(x=0.0, y=0.0, z=0.0, imp=np.nan, location='loc', filtering='filt', group=e_group)
         electrodes = self.nwbfile.create_electrode_table_region(region=[0], description='')
         e_series = ElectricalSeries(
             name='test_es',
@@ -283,9 +283,11 @@ def test_electrode_id_uniqueness(self):
                                                       description='',
                                                       location='',
                                                       device=device)
-        self.nwbfile.add_electrode(id=0, x=0.0, y=0.0, z=0.0, imp=np.nan, location='', filtering='', group=e_group)
+        self.nwbfile.add_electrode(id=0, x=0.0, y=0.0, z=0.0, imp=np.nan,
+                                   location='loc', filtering='filt', group=e_group)
         with self.assertRaises(ValueError):
-            self.nwbfile.add_electrode(id=0, x=0.0, y=0.0, z=0.0, imp=np.nan, location='', filtering='', group=e_group)
+            self.nwbfile.add_electrode(id=0, x=0.0, y=0.0, z=0.0, imp=np.nan, location='loc',
+                                       filtering='filt', group=e_group)
 
 
 class TestH5DataIO(TestCase):

diff --git a/tests/integration/hdf5/test_misc.py b/tests/integration/hdf5/test_misc.py
@@ -89,9 +89,7 @@ def addContainer(self, nwbfile):
                                                          device=device)
         for idx in [1, 2, 3, 4]:
             nwbfile.add_electrode(id=idx,
-                                  x=1.0, y=2.0, z=3.0,
-                                  imp=float(-idx),
-                                  location='CA1', filtering='none',
+                                  location='CA1',
                                   group=electrode_group)
 
         nwbfile.add_unit(id=1, electrodes=[1], electrode_group=electrode_group)
@@ -151,8 +149,7 @@ def make_electrode_table(self):
                                     location='tetrode location',
                                     device=self.dev1)
         for i in range(4):
-            self.table.add_row(x=float(i), y=2.0, z=3.0, imp=-1.0, location='CA1', filtering='none', group=self.group,
-                               group_name='tetrode1')
+            self.table.add_row(location='CA1', group=self.group, group_name='tetrode1')
 
     def setUpContainer(self):
         """ Return the test ElectricalSeries to read/write """

diff --git a/tests/unit/test_ecephys.py b/tests/unit/test_ecephys.py
@@ -15,14 +15,11 @@ def make_electrode_table():
     table = ElectrodeTable()
     dev1 = Device('dev1')
     group = ElectrodeGroup('tetrode1', 'tetrode description', 'tetrode location', dev1)
-    table.add_row(id=1, x=1.0, y=2.0, z=3.0, imp=-1.0, location='CA1', filtering='none',
-                  group=group, group_name='tetrode1')
-    table.add_row(id=2, x=1.0, y=2.0, z=3.0, imp=-2.0, location='CA1', filtering='none',
-                  group=group, group_name='tetrode1')
-    table.add_row(id=3, x=1.0, y=2.0, z=3.0, imp=-3.0, location='CA1', filtering='none',
-                  group=group, group_name='tetrode1')
-    table.add_row(id=4, x=1.0, y=2.0, z=3.0, imp=-4.0, location='CA1', filtering='none',
-                  group=group, group_name='tetrode1')
+    table.add_row(location='CA1', group=group, group_name='tetrode1')
+    table.add_row(location='CA1', group=group, group_name='tetrode1')
+    table.add_row(location='CA1', group=group, group_name='tetrode1')
+    table.add_row(location='CA1', group=group, group_name='tetrode1')
+
     return table
 
 

diff --git a/tests/unit/test_file.py b/tests/unit/test_file.py
@@ -86,7 +86,7 @@ def test_create_electrode_group_invalid_index(self):
         device = nwbfile.create_device('a')
         elecgrp = nwbfile.create_electrode_group('a', 'b', device=device, location='a')
         for i in range(4):
-            nwbfile.add_electrode(np.nan, np.nan, np.nan, np.nan, 'a', 'a', elecgrp, id=i)
+            nwbfile.add_electrode(location='a', group=elecgrp, id=i)
         with self.assertRaises(IndexError):
             nwbfile.create_electrode_table_region(list(range(6)), 'test')
 
@@ -97,7 +97,7 @@ def test_access_group_after_io(self):
         nwbfile = NWBFile('a', 'b', datetime.now(tzlocal()))
         device = nwbfile.create_device('a')
         elecgrp = nwbfile.create_electrode_group('a', 'b', device=device, location='a')
-        nwbfile.add_electrode(np.nan, np.nan, np.nan, np.nan, 'a', 'a', elecgrp, id=0)
+        nwbfile.add_electrode(location='a', group=elecgrp, id=0)
 
         with NWBHDF5IO('electrodes_mwe.nwb', 'w') as io:
             io.write(nwbfile)
@@ -108,7 +108,7 @@ def test_access_group_after_io(self):
                 self.assertEqual(aa.name, bb.name)
 
         for i in range(4):
-            nwbfile.add_electrode(np.nan, np.nan, np.nan, np.nan, 'a', 'a', elecgrp, id=i + 1)
+            nwbfile.add_electrode(location='a', group=elecgrp, id=i + 1)
 
         with NWBHDF5IO('electrodes_mwe.nwb', 'w') as io:
             io.write(nwbfile)
@@ -189,14 +189,12 @@ def test_set_electrode_table(self):
         table = ElectrodeTable()
         dev1 = self.nwbfile.create_device('dev1')
         group = self.nwbfile.create_electrode_group('tetrode1', 'tetrode description', 'tetrode location', dev1)
-        table.add_row(x=1.0, y=2.0, z=3.0, imp=-1.0, location='CA1', filtering='none', group=group,
-                      group_name='tetrode1')
-        table.add_row(x=1.0, y=2.0, z=3.0, imp=-2.0, location='CA1', filtering='none', group=group,
-                      group_name='tetrode1')
-        table.add_row(x=1.0, y=2.0, z=3.0, imp=-3.0, location='CA1', filtering='none', group=group,
-                      group_name='tetrode1')
-        table.add_row(x=1.0, y=2.0, z=3.0, imp=-4.0, location='CA1', filtering='none', group=group,
-                      group_name='tetrode1')
+
+        table.add_row(location='CA1', group=group, group_name='tetrode1')
+        table.add_row(location='CA1', group=group, group_name='tetrode1')
+        table.add_row(location='CA1', group=group, group_name='tetrode1')
+        table.add_row(location='CA1', group=group, group_name='tetrode1')
+
         self.nwbfile.set_electrode_table(table)
 
         self.assertIs(self.nwbfile.electrodes, table)
@@ -305,6 +303,28 @@ def test_add_electrode_some_opt(self):
         self.assertEqual(elec.iloc[0]['rel_z'], 9.0)
         self.assertEqual(elec.iloc[0]['reference'], 'ref2')
 
+    def test_add_electrode_missing_location(self):
+        """
+        Test the case where the user creates an electrode table region with
+        indexes that are out of range of the amount of electrodes added.
+        """
+        nwbfile = NWBFile('a', 'b', datetime.now(tzlocal()))
+        device = nwbfile.create_device('a')
+        elecgrp = nwbfile.create_electrode_group('a', 'b', device=device, location='a')
+        msg = "The 'location' argument is required when creating an electrode."
+        with self.assertRaisesWith(ValueError, msg):
+            nwbfile.add_electrode(group=elecgrp, id=0)
+
+    def test_add_electrode_missing_group(self):
+        """
+        Test the case where the user creates an electrode table region with
+        indexes that are out of range of the amount of electrodes added.
+        """
+        nwbfile = NWBFile('a', 'b', datetime.now(tzlocal()))
+        msg = "The 'group' argument is required when creating an electrode."
+        with self.assertRaisesWith(ValueError, msg):
+            nwbfile.add_electrode(location='a', id=0)
+
     def test_all_children(self):
         ts1 = TimeSeries('test_ts1', [0, 1, 2, 3, 4, 5], 'grams', timestamps=[0.0, 0.1, 0.2, 0.3, 0.4, 0.5])
         ts2 = TimeSeries('test_ts2', [0, 1, 2, 3, 4, 5], 'grams', timestamps=[0.0, 0.1, 0.2, 0.3, 0.4, 0.5])
@@ -358,8 +378,8 @@ def test_copy(self):
         self.nwbfile.add_unit(spike_times=[1., 2., 3.])
         device = self.nwbfile.create_device('a')
         elecgrp = self.nwbfile.create_electrode_group('a', 'b', device=device, location='a')
-        self.nwbfile.add_electrode(np.nan, np.nan, np.nan, np.nan, 'a', 'a', elecgrp, id=0)
-        self.nwbfile.add_electrode(np.nan, np.nan, np.nan, np.nan, 'b', 'b', elecgrp)
+        self.nwbfile.add_electrode(x=1.0, location='a', group=elecgrp, id=0)
+        self.nwbfile.add_electrode(x=2.0, location='b', group=elecgrp)
         elec_region = self.nwbfile.create_electrode_table_region([1], 'name')
 
         ts1 = TimeSeries('test_ts1', [0, 1, 2, 3, 4, 5], 'grams', timestamps=[0.0, 0.1, 0.2, 0.3, 0.4, 0.5])

diff --git a/tests/unit/test_misc.py b/tests/unit/test_misc.py
@@ -84,8 +84,7 @@ def make_electrode_table(self):
                                     location='tetrode location',
                                     device=self.dev1)
         for i in range(4):
-            self.table.add_row(x=i, y=2.0, z=3.0, imp=-1.0, location='CA1', filtering='none', group=self.group,
-                               group_name='tetrode1')
+            self.table.add_row(location='CA1', group=self.group, group_name='tetrode1')
 
     def test_init_with_source_channels(self):
         self.make_electrode_table(self)