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

Dynamic table unique ids #316

Merged
merged 9 commits into from
Nov 30, 2022
Merged

Dynamic table unique ids #316

merged 9 commits into from
Nov 30, 2022

Conversation

bendichter
Copy link
Contributor

Motivation

What was the reasoning behind this change? Please explain the changes briefly.

@CodyCBakerPhD CodyCBakerPhD added the category: new check a new best practices check to apply to all NWBFiles and their contents label Nov 30, 2022
@CodyCBakerPhD
Copy link
Contributor

LGTM - did you check this against the NeuroNexus file?

@bendichter
Copy link
Contributor Author

LGTM - did you check this against the NeuroNexus file?

yup., it catches it

@CodyCBakerPhD
Copy link
Contributor

yup., it catches it

Great, can you write or update a best practices section in the docs for it as well? In that section it can be mentioned that the easiest solution is often to try updating the PyNWB version. For custom DynamicTables obviously the user has full control over that kind of stuff, but this violation came from something that old PyNWB versions generated automatically through convenience functions like the first call to add_electrodes

@CodyCBakerPhD
Copy link
Contributor

Codecov v3 has had problems uploading on occasion - feel free to ignore and merge or otherwise retrigger it and hope for the best

@bendichter bendichter merged commit 4bd63f9 into dev Nov 30, 2022
@CodyCBakerPhD CodyCBakerPhD deleted the dynamic_table_unique_ids branch December 2, 2022 03:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: new check a new best practices check to apply to all NWBFiles and their contents
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants