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

[Repeal Best Practice]: TimeSeries Subtypes #327

Open
CodyCBakerPhD opened this issue Dec 12, 2022 · 1 comment
Open

[Repeal Best Practice]: TimeSeries Subtypes #327

CodyCBakerPhD opened this issue Dec 12, 2022 · 1 comment
Labels
category: proposal proposed enhancements or new features

Comments

@CodyCBakerPhD
Copy link
Contributor

The Best Practice in question: https://nwbinspector.readthedocs.io/en/dev/best_practices/time_series.html#subtypes sates

ElectricalSeries are reserved for neural data. An ElectricalSeries holds signal from electrodes positioned in or around the brain that are monitoring neural activity, and only those electrodes should be in the ElectrodeTable.

For non-neural electrodes that still may store and report raw values in Volts, simply use a general TimeSeries object with units set to “Volts”.

I would like to open a discussion @oruebel @rly @bendichter about relaxing this as a Best Practice.

The main reasoning for this practice is because an ElectricalSeries must link to electrodes and the current electrodes table has many required fields (and many more optional fields) that reflect neurophysiological properties, such as brain areas, probe geometry, and so on. Thus for analog, digital, environmental, or other auxiliary types of electrodes, such columns would be completely inapplicable and so they don't 'belong' on the main electrodes table and hence should not be written using ElectricalSeries.

However, as ecephys systems have been increasingly developed to contain non-neural channel activity, we've seen many cases where users, and indeed fellow devs, are motivated to use the ElectricalSeries type to write data from channels that are recording only secondary (non-neurophysiological) data.

This best practice is not obvious upon first glance why it should prohibit the use of this neurodata type, since the data we want to represent is stored in Volts, and has metadata structure nearly identical to the Device/ElectrodeGroup/Electrodes hierarchy established by the ElectricalSeries. In fact, for many recording systems, the data and metadata for those channels are stored side-by-side and nearly indistinguishable from the neurophysiological ones.

If we follow the best practice, we lose a lot of rich metadata linkage between the acquisition device, channel/group relations, and even non-neural electrode channel metadata because the resulting TimeSeries cannot link to any of these. As such I'd like to propose a couple of solutions that work around the reasoning for the Best Practice, and would result in repealing or adjusting it as a practice going into the future.

Proposed interim solution:

This workflow ought to be possible currently since I don't think ElectricalSeries checks if the DynamicTableRegion of the electrodes field points to an actual electrodes table...

  • Create a custom DynamicTable that mimics much of the structure of the usual electrodes table but foregoes the neurophysiogical columns. This is added to acquisition.
  • Write separate ElectricalSeries for the non-neural electrode data and link them to the custom DynamicTable

Proposed long-term solution:

  • Create a BaseElectrodeTable neurodata type that defines the minimal columns for all types of electrodes.
  • Make the main electrodes table an actual neurodata type inheriting from that base. Work has partially begun on this in PyNWB PR #1539. I'd also vote to name this new neurodata type a NeurophysiologyElectrodeTable, or any other name that distinguishes it from non-neural electrodes. Note that all current nwbfile level references can still exist (such as nwbfile.electrodes, nwbfile.add_electrode, etc.) without breaking any back-compatibility.
  • Add a new separate subclass of that base type called something like AuxiliaryElectrodeTable which does not contain those neural-related fields. ElectricalSeries.electrodes table regions may point to either this table or the neurophysiologic one - all that matter is the DynamicTableRegion points to some child of the BaseElectrodeTable.
@CodyCBakerPhD CodyCBakerPhD added the category: proposal proposed enhancements or new features label Dec 12, 2022
@CodyCBakerPhD CodyCBakerPhD self-assigned this Dec 12, 2022
@rly
Copy link
Contributor

rly commented Dec 13, 2022

Thanks @CodyCBakerPhD for the detailed background and proposal.

I agree that the current best practice is not ideal, and I like your proposed long-term solution.

To contrast, here is another possible solution:
Create a new type AuxiliaryElectricalSeries that has the same fields as ElectricalSeries and links to an AuxiliaryElectrodeTable which does not contain neural-related fields. Perhaps this AuxiliaryElectrodeTable can have a column for an AuxiliaryElectrodeGroup which also does not contain neural-related fields.

  • Pros: creates separate types for different kinds of data that may have different applications/uses. It is also less complex.
  • Cons: duplicates functionality between the electrical series, the electrodes table, and the electrode group data types.

I think the type inheritance model that you proposed is probably better and cleaner.

@CodyCBakerPhD CodyCBakerPhD removed their assignment Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: proposal proposed enhancements or new features
Projects
None yet
Development

No branches or pull requests

2 participants