NeurodataWithoutBorders / nwbinspector

Tool to help inspect NWB files for compliance with NWB Best Practices
https://nwbinspector.readthedocs.io/
Other
17 stars 10 forks source link

[Repeal Best Practice]: TimeSeries Subtypes #327

Open CodyCBakerPhD opened 1 year ago

CodyCBakerPhD commented 1 year ago

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...

Proposed long-term solution:

rly commented 1 year ago

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.

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