Repository: NeurodataWithoutBorders/pynwb

Issue:
[Bug]: Writing ElectrodeGroup position saves incorrectly modified data to file
### What happened?

There seem to be some inconsistencies in writing ElectrodeGroup position to a file depending on the input type:
* For `np.ndarray` inputs, the `__init__` function checks whether position was provided using `if position` instead of `if position is not None`, which leads to the usual `The truth value of an array with more than one element is ambiguous` error
* For tuple inputs, the saved `position` data seems to be duplicated into an array of tuples like `[(x, y, z), (x, y, z), (x, y, z)]`
* For list inputs, the saved `position` data seems to be duplicated into a array of tuples like `[(x, x, x), (y, y, y), (z, z, z)]`

These last two things only happen on the write/read round trip, not before writing the file.

### Steps to Reproduce

```python
import numpy as np
import os
from pynwb import NWBFile, NWBHDF5IO
from uuid import uuid4
from datetime import datetime
​
nwbfile = NWBFile(
    session_description="no description.",
    session_start_time=datetime.now(),
    identifier=str(uuid4()),
)
​
device = nwbfile.create_device(name="Device", description="no description.")
​
electrode_group = nwbfile.create_electrode_group(
    name="ElectrodeGroup",
    description="no description.",
    device=device,
    location="unknown",
    # position=np.array([1., 2., 3.]),
    position=(1., 2., 3.),
    # position=[1., 2., 3.],
)
​
with NWBHDF5IO('test.nwb', 'w') as io:
    io.write(nwbfile)
    
with NWBHDF5IO('test.nwb', 'r') as io:
    read_nwbfile = io.read()
    
    print(read_nwbfile.electrode_groups['ElectrodeGroup'])
    print(read_nwbfile.electrode_groups['ElectrodeGroup'].position[()])
```


### Traceback

```python
# for position = np.array([1., 2., 3.])
Traceback (most recent call last):
  File "/home/jovyan/scripts/test_write_electrode_group_position.py", line 16, in <module>
    electrode_group = nwbfile.create_electrode_group(
  File "/opt/conda/lib/python3.10/site-packages/hdmf/utils.py", line 644, in func_call
    return func(args[0], **pargs)
  File "/opt/conda/lib/python3.10/site-packages/hdmf/container.py", line 1044, in _func
    ret = container_type(**kwargs)
  File "/opt/conda/lib/python3.10/site-packages/hdmf/utils.py", line 644, in func_call
    return func(args[0], **pargs)
  File "/opt/conda/lib/python3.10/site-packages/pynwb/ecephys.py", line 33, in __init__
    if args_to_set['position'] and len(args_to_set['position']) != 3:
ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

# for position=(1., 2., 3.)
ElectrodeGroup pynwb.ecephys.ElectrodeGroup at 0x140662980868848
Fields:
  description: no description.
  device: Device pynwb.device.Device at 0x140662980868752
Fields:
  description: no description.

  location: unknown
  position: <hdmf.backends.hdf5.h5_utils.ContainerH5TableDataset object at 0x7feea701fb50>

[(1., 2., 3.) (1., 2., 3.) (1., 2., 3.)]

# for position=[1., 2., 3.]
ElectrodeGroup pynwb.ecephys.ElectrodeGroup at 0x140659597409008
Fields:
  description: no description.
  device: Device pynwb.device.Device at 0x140659597408912
Fields:
  description: no description.

  location: unknown
  position: <hdmf.backends.hdf5.h5_utils.ContainerH5TableDataset object at 0x7feddd567b50>

[(1., 1., 1.) (2., 2., 2.) (3., 3., 3.)]
```


### Operating System

Linux

### Python Executable

Python

### Python Version

3.10

### Package Versions

[issue_environment.txt](https://github.com/NeurodataWithoutBorders/pynwb/files/12457610/issue_environment.txt)

### Code of Conduct

- [X] I agree to follow this project's [Code of Conduct](https://github.com/NeurodataWithoutBorders/pynwb/blob/dev/.github/CODE_OF_CONDUCT.rst)
- [X] Have you checked the [Contributing](https://github.com/NeurodataWithoutBorders/pynwb/blob/dev/docs/CONTRIBUTING.rst) document?
- [X] Have you ensured this bug was not already [reported](https://github.com/NeurodataWithoutBorders/pynwb/issues)?

Discussion:
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/NeurodataWithoutBorders/pynwb/pull/1770?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NeurodataWithoutBorders) Report
> Merging [#1770](https://app.codecov.io/gh/NeurodataWithoutBorders/pynwb/pull/1770?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NeurodataWithoutBorders) (dc8c1c4) into [dev](https://app.codecov.io/gh/NeurodataWithoutBorders/pynwb/commit/68c4f5645d00f3958471e5e2bc360c90f49a4740?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NeurodataWithoutBorders) (68c4f56) will **decrease** coverage by `8.29%`.
> The diff coverage is `92.85%`.

```diff
@@            Coverage Diff             @@
##              dev    #1770      +/-   ##
==========================================
- Coverage   91.98%   83.70%   -8.29%     
==========================================
  Files          27       27              
  Lines        2608     2620      +12     
  Branches      681      686       +5     
==========================================
- Hits         2399     2193     -206     
- Misses        134      339     +205     
- Partials       75       88      +13     
```

| [Flag](https://app.codecov.io/gh/NeurodataWithoutBorders/pynwb/pull/1770/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NeurodataWithoutBorders) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/NeurodataWithoutBorders/pynwb/pull/1770/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NeurodataWithoutBorders) | `?` | |
| [unit](https://app.codecov.io/gh/NeurodataWithoutBorders/pynwb/pull/1770/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NeurodataWithoutBorders) | `83.70% <92.85%> (+0.03%)` | :arrow_up: |

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=NeurodataWithoutBorders#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Files Changed](https://app.codecov.io/gh/NeurodataWithoutBorders/pynwb/pull/1770?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NeurodataWithoutBorders) | Coverage Δ | |
|---|---|---|
| [src/pynwb/ecephys.py](https://app.codecov.io/gh/NeurodataWithoutBorders/pynwb/pull/1770?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NeurodataWithoutBorders#diff-c3JjL3B5bndiL2VjZXBoeXMucHk=) | `96.74% <92.85%> (-0.55%)` | :arrow_down: |

... and [14 files with indirect coverage changes](https://app.codecov.io/gh/NeurodataWithoutBorders/pynwb/pull/1770/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NeurodataWithoutBorders)

:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NeurodataWithoutBorders)

oruebel: Just some detective work for background:

*  It seems that `ElectrodeGroup.position` was added in NWB v2.2 in #1146. I believe this bug has existed since the field was added.
* Looking at https://github.com/NeurodataWithoutBorders/nwb-schema/commit/59db28ee96cfe63f7304789cc879cd94a75632ce it seems that `ElectrodeGroup.position` was first added as an array of length 3 (which is what the current check is looking for) and then changed to a compound data type. I.e., my guess is that PyNWB didn't get updated when the change to compound happened, and the unit tests did not fully cover this field. 

Given that it has take 3 years for this issue to surface, it seems that `ElectrodeGroup.position` is probably rarely used. 
CodyCBakerPhD: @oruebel Any updates on this PR?
oruebel: @stephprince could you take a look at this PR. I think this is essentially complete but needs some final attention to get over the finish line. 

rly: @stephprince do you have a chance to take a look at this? thanks!
stephprince: Yes sorry for the delay I'll take a look!
rly: Ok, after doing a full review of the issue and PR, I am also confused as to why `ElectrodeGroup.position` is allowed to be a list of tuples. I think @oruebel may be mistaken. `ElectrodeGroup.position` should be a **scalar** dataset with compound dtype (x, y, z). @oruebel can you review?
oruebel: > I think @oruebel may be mistaken. `ElectrodeGroup.position` should be a **scalar** dataset with compound dtype (x, y, z)

The schema [here](https://github.com/NeurodataWithoutBorders/nwb-schema/blob/3ad7aa370907c2cc177d033065dcba52140ca5a7/core/nwb.ecephys.yaml#L242-L255) says:

```
  datasets:
  - name: position
    dtype:
    - name: x
      dtype: float32
      doc: x coordinate
    - name: y
      dtype: float32
      doc: y coordinate
    - name: z
      dtype: float32
      doc: z coordinate
    doc: stereotaxic or common framework coordinates
    quantity: '?'
```

I.e., position is a compound dataset but the shape is not specified. Looking at the schema language docs [here](https://schema-language.readthedocs.io/en/latest/description.html#shape), I believe this means that `shape: null` is assumed so that it should be scalar. So yes, it seems that this should indeed be a scalar with a compound data type. So this PR should be updated accordingly. 

Question, how does the input for scalar compound datasets need to look like? Looking at the source issue #1768 setting:

- `position=np.array([1., 2., 3.])` : results in an error due to the validation in ElectrodeGroup
- `position=(1., 2., 3.)` : is being written as `[(1., 2., 3.) (1., 2., 3.) (1., 2., 3.)]`
-  `position=[1., 2., 3.]` : is being written as `[(1., 1., 1.) (2., 2., 2.) (3., 3., 3.)]`

Specifying as `[(1, 2, 3), ])` as currently on the PR fixes the bad write. Is `position=[(1, 2, 3), ])` the right form and we need to updated the validation to check that only a single tuple is given or is `position=(1., 2., 3.)` the correct form and we need to dig deeper into write? 
stephprince: After some further investigation, it turned out that scalar datasets with a compound datatype were being written incorrectly so that the dataset was not scalar. These changes should be addressed in the upcoming hdmf release (3.14.4), but the tests will fail until then.

With the hdmf update, I believe `ElectrodeGroup.position` validation should check whether the position is a scalar with compound dtype, or whether it’s a single list/tuple of length 3 that we then convert to a scalar with compound dtype. 
rly: This looks good to me but I will let @stephprince review and approve to merge.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index e5909f57..16e50465 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -5,6 +5,9 @@
 ### Performance
 - Cache global type map to speed import 3X. @sneakers-the-rat [#1931](https://github.com/NeurodataWithoutBorders/pynwb/pull/1931)
 
+### Bug fixes
+- Fixed bug in how `ElectrodeGroup.__init__` validates its `position` argument. @oruebel [#1770](https://github.com/NeurodataWithoutBorders/pynwb/pull/1770)
+
 ## PyNWB 2.8.2 (September 9, 2024)
 
 ### Enhancements and minor changes
diff --git a/docs/source/conf.py b/docs/source/conf.py
index 4eaf1a19..eabca22c 100644
--- a/docs/source/conf.py
+++ b/docs/source/conf.py
@@ -244,7 +244,6 @@ pygments_style = 'sphinx'
 # html_theme = 'default'
 # html_theme = "sphinxdoc"
 html_theme = "sphinx_rtd_theme"
-html_theme_path = [sphinx_rtd_theme.get_html_theme_path()]
 
 # Theme options are theme-specific and customize the look and feel of a theme
 # further.  For a list of options available for each theme, see the
@@ -260,9 +259,6 @@ html_css_files = [
     'css/custom.css',
 ]
 
-# Add any paths that contain custom themes here, relative to this directory.
-# html_theme_path = []
-
 # The name for this set of Sphinx documents.  If None, it defaults to
 # "<project> v<release> documentation".
 # html_title = None
diff --git a/pyproject.toml b/pyproject.toml
index f798f2b5..14a11f5d 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -34,7 +34,7 @@ classifiers = [
 ]
 dependencies = [
     "h5py>=2.10",
-    "hdmf>=3.14.3",
+    "hdmf>=3.14.5",
     "numpy>=1.18",
     "pandas>=1.1.5",
     "python-dateutil>=2.7.3",
diff --git a/requirements-min.txt b/requirements-min.txt
index eef051b2..feed604b 100644
--- a/requirements-min.txt
+++ b/requirements-min.txt
@@ -1,6 +1,6 @@
 # minimum versions of package dependencies for installing PyNWB
 h5py==2.10  # support for selection of datasets with list of indices added in 2.10
-hdmf==3.14.3
+hdmf==3.14.5
 numpy==1.18
 pandas==1.1.5
 python-dateutil==2.7.3
diff --git a/requirements.txt b/requirements.txt
index 6d7a1762..1e7a5e18 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -1,6 +1,6 @@
 # pinned dependencies to reproduce an entire development environment to use PyNWB
 h5py==3.11.0
-hdmf==3.14.3
+hdmf==3.14.5
 numpy==2.1.1; python_version > "3.9"  # numpy 2.1+ is not compatible with py3.9
 numpy==2.0.2; python_version == "3.9"
 pandas==2.2.2
diff --git a/src/pynwb/ecephys.py b/src/pynwb/ecephys.py
index 3187cca4..e6dadbe9 100644
--- a/src/pynwb/ecephys.py
+++ b/src/pynwb/ecephys.py
@@ -1,4 +1,5 @@
 import warnings
+import numpy as np
 from collections.abc import Iterable
 
 from hdmf.common import DynamicTableRegion
@@ -26,13 +27,31 @@ class ElectrodeGroup(NWBContainer):
             {'name': 'location', 'type': str, 'doc': 'description of location of this electrode group'},
             {'name': 'device', 'type': Device, 'doc': 'the device that was used to record from this electrode group'},
             {'name': 'position', 'type': 'array_data',
-             'doc': 'stereotaxic position of this electrode group (x, y, z)', 'default': None})
+             'doc': 'Compound dataset with stereotaxic position of this electrode group (x, y, z). '
+                    'The data array must have three elements or the dtype of the '
+                    'array must be ``(float, float, float)``', 'default': None})
     def __init__(self, **kwargs):
         args_to_set = popargs_to_dict(('description', 'location', 'device', 'position'), kwargs)
         super().__init__(**kwargs)
-        if args_to_set['position'] and len(args_to_set['position']) != 3:
-            raise ValueError('ElectrodeGroup position argument must have three elements: x, y, z, but received: %s'
-                             % str(args_to_set['position']))
+
+        # position is a compound dataset, i.e., this must be a scalar with a
+        # compound data type of three floats or a list/tuple of three entries
+        position = args_to_set['position']
+        if position:
+            # check position argument is valid
+            position_dtype_invalid = (
+                (hasattr(position, 'dtype') and len(position.dtype) != 3) or
+                (not hasattr(position, 'dtype') and len(position) != 3) or
+                (len(np.shape(position)) > 1)
+            )
+            if position_dtype_invalid:
+                raise ValueError(f"ElectrodeGroup position argument must have three elements: x, y, z,"
+                                 f"but received: {position}")
+
+            # convert position to scalar with compound data type if needed
+            if not hasattr(position, 'dtype'):
+                args_to_set['position'] = np.array(tuple(position), dtype=[('x', float), ('y', float), ('z', float)])
+
         for key, val in args_to_set.items():
             setattr(self, key, val)
 


Test patch:
diff --git a/tests/integration/hdf5/test_ecephys.py b/tests/integration/hdf5/test_ecephys.py
index ff67d27c..c4472527 100644
--- a/tests/integration/hdf5/test_ecephys.py
+++ b/tests/integration/hdf5/test_ecephys.py
@@ -26,7 +26,8 @@ class TestElectrodeGroupIO(NWBH5IOMixin, TestCase):
         eg = ElectrodeGroup(name='elec1',
                             description='a test ElectrodeGroup',
                             location='a nonexistent place',
-                            device=self.dev1)
+                            device=self.dev1,
+                            position=(1., 2., 3.))
         return eg
 
     def addContainer(self, nwbfile):
diff --git a/tests/unit/test_ecephys.py b/tests/unit/test_ecephys.py
index dc194af2..1415c3d3 100644
--- a/tests/unit/test_ecephys.py
+++ b/tests/unit/test_ecephys.py
@@ -178,16 +178,34 @@ class ElectrodeGroupConstructor(TestCase):
 
     def test_init(self):
         dev1 = Device('dev1')
-        group = ElectrodeGroup('elec1', 'electrode description', 'electrode location', dev1, (1, 2, 3))
+        group = ElectrodeGroup(name='elec1',
+                               description='electrode description',
+                               location='electrode location',
+                               device=dev1,
+                               position=(1, 2, 3))
         self.assertEqual(group.name, 'elec1')
         self.assertEqual(group.description, 'electrode description')
         self.assertEqual(group.location, 'electrode location')
         self.assertEqual(group.device, dev1)
-        self.assertEqual(group.position, (1, 2, 3))
+        self.assertEqual(group.position.tolist(), (1, 2, 3))
+
+    def test_init_position_array(self):
+        position = np.array((1, 2, 3), dtype=np.dtype([('x', float), ('y', float), ('z', float)]))
+        dev1 = Device('dev1')
+        group = ElectrodeGroup('elec1', 'electrode description', 'electrode location', dev1,
+                               position)
+        self.assertEqual(group.name, 'elec1')
+        self.assertEqual(group.description, 'electrode description')
+        self.assertEqual(group.location, 'electrode location')
+        self.assertEqual(group.device, dev1)
+        self.assertEqual(group.position, position)
 
     def test_init_position_none(self):
         dev1 = Device('dev1')
-        group = ElectrodeGroup('elec1', 'electrode description', 'electrode location', dev1)
+        group = ElectrodeGroup(name='elec1',
+                               description='electrode description',
+                               location='electrode location',
+                               device=dev1)
         self.assertEqual(group.name, 'elec1')
         self.assertEqual(group.description, 'electrode description')
         self.assertEqual(group.location, 'electrode location')
@@ -197,7 +215,29 @@ class ElectrodeGroupConstructor(TestCase):
     def test_init_position_bad(self):
         dev1 = Device('dev1')
         with self.assertRaises(ValueError):
-            ElectrodeGroup('elec1', 'electrode description', 'electrode location', dev1, (1, 2))
+            ElectrodeGroup(name='elec1',
+                           description='electrode description',
+                           location='electrode location',
+                           device=dev1,
+                           position=(1, 2))
+        with self.assertRaises(ValueError):
+            ElectrodeGroup(name='elec1',
+                           description='electrode description',
+                           location='electrode location',
+                           device=dev1,
+                           position=[(1, 2), ])
+        with self.assertRaises(ValueError):
+            ElectrodeGroup(name='elec1',
+                           description='electrode description',
+                           location='electrode location',
+                           device=dev1,
+                           position=np.array([(1., 2.)], dtype=np.dtype([('x', float), ('y', float)])))
+        with self.assertRaises(ValueError):
+            ElectrodeGroup(name='elec1',
+                           description='electrode description',
+                           location='electrode location',
+                           device=dev1,
+                           position=[(1, 2, 3), (4, 5, 6), (7, 8, 9)])
 
 
 class EventDetectionConstructor(TestCase):