Add unique electrode id check

[CodyCBakerPhD]

fix #1342

Motivation

While trying to solve this issue on spikeextractors, I ran across this problem as a contributing factor. The docstring and overall schema treatment of electrode table IDs mandates they be unique, but we can set them arbitrarily through pynwb.

The new changes use the add_row functionality in DynamicTables enforce unique electrode ids as per @oruebel's suggestion. To avoid decreases to computational performance/speed, this only triggers if the user manually specified the ids (those set automatically by default do not need to be checked).

How to test the behavior?

import numpy as np
import uuid
from datetime import datetime

import spikeextractors as se
from pynwb import NWBFile

nwbfile_kwargs = dict(
    session_description="Auto-generated by NwbRecordingExtractor without description.",
    identifier=str(uuid.uuid4()),
    session_start_time=datetime(1970, 1, 1)
)
nwbfile = NWBFile(**nwbfile_kwargs)

re = se.example_datasets.toy_example()[0]

se.NwbRecordingExtractor.add_devices(recording=re, nwbfile=nwbfile)
se.NwbRecordingExtractor.add_electrode_groups(recording=re, nwbfile=nwbfile)

defaults = dict(
    x=np.nan,
    y=np.nan,
    z=np.nan,
    # There doesn't seem to be a canonical default for impedence, if missing.
    # The NwbRecordingExtractor follows the -1.0 convention, other scripts sometimes use np.nan
    imp=-1.0,
    location="unknown",
    filtering="none",
    group_name="Electrode Group"
)
nwbfile.add_electrode(**dict(defaults, id=0, group=nwbfile.electrode_groups[str(0)]))
# nwbfile.add_electrode(**dict(defaults, id=0, group=nwbfile.electrode_groups[str(0)]))
nwbfile.add_electrode(**dict(defaults, group=nwbfile.electrode_groups[str(0)]))

Uncommenting the second to last line will trigger the new uniqueness error. There is also a test in test_io.py that checks this behavior.

Checklist

• Have you checked our Contributing document?
• Have you ensured the PR description clearly describes problem and the solution?
• Is your contribution compliant with our coding style? This can be checked running flake8 from the source directory.
• Have you checked to ensure that there aren't other open Pull Requests for the same change?
• Have you included the relevant issue number using #XXX notation where XXX is the issue number?

[rly]

I think checking the electrode ID is unique should be the default, regardless of whether the user is manually or automatically setting it. Imagine if the user adds an electrode with manual ID = 1. Then adds an electrode with automatic ID. DynamicTable.add_row would set that ID to be len(self.id) which is 1 and creates a duplicate.

Instead of only triggering if the user manually specified the ids, what do you think about adding a flag enforce_unique_ids to add_electrode(...), just like in DynamicTable.add_row(...), but instead of default=False, the default value is True? This gives the user more control over when the check is made.

i.e.

@docval(..., 
	    {'name': 'enforce_unique_id', 'type': bool, 'doc': 'enforce that the id in the table must be unique',
	    'default': True})
def add_electrode(self, **kwargs):

There would be an added computational cost since this would by default run every time an electrode is added. Currently the uniqueness check is not handled efficiently in NWB/HDMF. One way to do this efficiently is implementing a btree/hash-based index in DynamicTable for lookup by ID, similar to how some databases maintain uniqueness.

[CodyCBakerPhD]

Good point on the default case, I didn't think to check how the auto-population was encoded.

I think that is a great idea to just add it to the docval - I've added that now.

The computational cost isn't truly a concern at the moment (it's hardly the most expensive thing to convert to NWB) but was just an attempt at optimizing the code as much as possible. I agree, though, that more sophisticated methods of uniqueness checking could make it a complete non-issue.

[CodyCBakerPhD]

@rly Any other changes you would like me to make here?

[oruebel]

Please add a not to the changelog.md, otherwise this looks good to me. This PR changes default API behavior, but since the next release is going to a major version bump I don't see any issues.

[CodyCBakerPhD]

@oruebel @rly Changelog updated

[rly]

Closing and reopening to try to trigger CI.

[rly]

Thanks @CodyCBakerPhD !