Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add_electrode does not check uniquess of id #1342

Closed
5 tasks done
CodyCBakerPhD opened this issue Mar 2, 2021 · 3 comments · Fixed by #1344
Closed
5 tasks done

add_electrode does not check uniquess of id #1342

CodyCBakerPhD opened this issue Mar 2, 2021 · 3 comments · Fixed by #1344
Labels
category: bug errors in the code or code behavior

Comments

@CodyCBakerPhD
Copy link
Collaborator

Description

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.

Steps to Reproduce

import numpy as np
import uuid
from datetime import datetime

import spikeextractors as se
from pynwb import NWBFile

nwbfile_kwargs = dict(
    session_description="Testing NWBFile",
    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)]))

This should throw an error letting the user know to make the ids unique so as not to cause confusion for downstream mapping through the tables.

Environment

Python Executable: Python
Python Version: Python 3.8
Operating System: Windows
HDMF Version: 2.4.0
PyNWB Version: 1.4.0

Checklist

  • Have you ensured the feature or change was not already reported?
  • Have you included a brief and descriptive title?
  • Have you included a clear description of the problem you are trying to solve?
  • Have you included a minimal code snippet that reproduces the issue you are encountering?
  • Have you checked our Contributing document?
@CodyCBakerPhD CodyCBakerPhD added the category: bug errors in the code or code behavior label Mar 2, 2021
@CodyCBakerPhD
Copy link
Collaborator Author

I can try submitting a PR for this myself in the next couple days, if not already solved by then.

@oruebel
Copy link
Contributor

oruebel commented Mar 2, 2021

DynamicTable.add_row has a feature to enforce unique id's when adding items to a table https://hdmf.readthedocs.io/en/stable/hdmf.common.table.html#hdmf.common.table.DynamicTable.add_row I'm not sure, but it may be as simple as setting this flag when updating the table to enforce the unique id's here.

@CodyCBakerPhD
Copy link
Collaborator Author

@oruebel That does look like a very simple solution, I'll try it out! Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: bug errors in the code or code behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants