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

Define ElectrodesTable and FrequencyBandsTable types #337

Draft
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

rly
Copy link
Contributor

@rly rly commented Nov 27, 2019

Fix #336

oruebel
oruebel previously approved these changes Nov 27, 2019
@bendichter
Copy link
Contributor

This won't cause validation or IO errors for files that already exist? If this can be done without disrupting users I think it's a good idea

@oruebel
Copy link
Contributor

oruebel commented Nov 27, 2019

I think on the PyNWB the validation and read of existing files can be handled. @bendichter does this change anything for MatNWB?

@bendichter
Copy link
Contributor

@oruebel I don't know, possibly. I can check

@bendichter
Copy link
Contributor

I'm having trouble getting the matnwb to work. They are several schema changes behind at this point.

@bendichter
Copy link
Contributor

bendichter commented Dec 27, 2019

This break matnwb/tutorials/ecephys.m because the electrodes table is created manually as a DynamicTable, which needs to change to an Electrodes object. I'll fix that and see if there are any other issues

@bendichter
Copy link
Contributor

I would prefer to have the convention that tables are called Tables, i.e. this would be an ElectrodesTable object, similar to how we name descendants of TimeSeries with Series

@bendichter
Copy link
Contributor

@rly status update: this is working on the matnwb side

@rly
Copy link
Contributor Author

rly commented Jan 2, 2020

I would prefer to have the convention that tables are called Tables, i.e. this would be an ElectrodesTable object, similar to how we name descendants of TimeSeries with Series

I would prefer this convention as well. However, we already have the Units table and the TimeIntervals table (which can have fixed names "epochs", "trials", "invalid_times"). The icephys extension will also add a few more DynamicTable types: IntracellularRecordings, SweepSequences, Runs, and Conditions.

@bendichter
Copy link
Contributor

bendichter commented Jan 2, 2020

I was talking about a convention for the neurodata_type name, not the object name, so it's not an issue for "epochs", "trials", "invalid_times". I agree we have types that break this convention, but I think we should use it going forward. At least when I create extensions I am going to try to use this naming convention for children of DynamicTable. At any rate, this schema change looks good to me besides that.

@rly
Copy link
Contributor Author

rly commented Jan 3, 2020

I renamed the types and suggested a renaming of the icephys extension types: oruebel/ndx-icephys-meta#27

If we go forward with this convention, I also suggest renaming Units to UnitsTable and making Units an alias to UnitsTable to maintain backwards compatibility. Same for TimeIntervals to TimeIntervalsTable.

@bendichter
Copy link
Contributor

@rly that sounds scary. I'm worried about backwards compatibility, esp for matnwb

@rly rly added this to the NWB 2.3.0 milestone Jun 1, 2020
@rly rly modified the milestones: NWB 2.3.0, NWB 2.4.0 Aug 6, 2020
@rly rly changed the title Define Electrodes and FrequencyBands types Define ElectrodesTable and FrequencyBandsTable types Jul 21, 2021
@rly rly modified the milestones: NWB 2.4.0, NWB 2.5.0 Aug 5, 2021
@rly rly marked this pull request as draft August 5, 2021 20:20
@stephprince stephprince removed this from the NWB 2.5.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Electrodes table and frequency bands table should be their own types
4 participants