AIBS_ecephys NWB extension suggestions

[bendichter]

Here are a few suggestions for you extension:

• AllenSDK/allensdk/brain_observatory/ecephys/nwb/AIBS_ecephys_extension.yaml

  Lines 2 to 3 in a040661

  	- neurodata_type_def: EcephysProbe
  	neurodata_type_inc: ElectrodeGroup

  
  I would categorize a neuropixel EcephysProbe as a Device, not an ElectrodeGroup.

• AllenSDK/allensdk/brain_observatory/ecephys/nwb/AIBS_ecephys_extension.yaml

  Lines 6 to 8 in a040661

  	- name: help
  	dtype: text
  	value: A physical grouping of channels

  
  The "help" attribute is no longer required, so you may wish to remove it.

• AllenSDK/allensdk/brain_observatory/ecephys/nwb/AIBS_ecephys_extension.yaml

  Lines 16 to 18 in a040661

  	- name: sampling_rate
  	dtype: float64
  	doc: the sampling rate at which data were acquired on this probe's channels

  
  We generally record the sampling rate as part of the ElectricalSeries, not the probe. This attribute is really a feature of the voltage measurements themselves. Also, it is possible for a probe to collect data at varying sampling rates.

• AllenSDK/allensdk/brain_observatory/ecephys/nwb/AIBS_ecephys_extension.yaml

  Lines 19 to 21 in a040661

  	- name: lfp_sampling_rate
  	dtype: float64
  	doc: the (probably reduced) sampling rate at which lfp data were acquired on this probe's channels

  
  We would generally keep track of the LFP sampling rate in /processing/ecephys/LFP/ElectricalSeries/starting_time.rate. Again, this is really a feature of that data, not of the neuropixel probe. You choose when you downsample what the resulting rate should be, so it is very feasible to have different LFP sampling rates for the same probe.

• AllenSDK/allensdk/brain_observatory/ecephys/nwb/AIBS_ecephys_extension.yaml

  Lines 29 to 50 in a040661

  	- neurodata_type_def: EcephysLabMetaData
  	neurodata_type_inc: LabMetaData
  	doc: "metadata for ecephys sessions"
  	attributes:
  	- name: specimen_name
  	dtype: text
  	doc: full name of this specimen
  	- name: age_in_days
  	dtype: float
  	doc: age of this subject, in days
  	- name: full_genotype
  	dtype: text
  	doc: "long-form description of this subjects genotype"
  	- name: strain
  	dtype: text
  	doc: "this subjects strain"
  	- name: sex
  	dtype: text
  	doc: "this subjects sex"
  	- name: stimulus_name
  	dtype: text
  	doc: "the name of the stimulus set used for this session"

  
  You have included subject information in your EcephysLabMetaData object. This information would be better in Subject. In fact, there are already fields for "sex", "strain", "genotype", and "date_of_birth" in the Subject object. If you want to pre-compute the 'age_in_days" and provide that as an attribute, the best way to do that would be to extend Subject.

• AllenSDK/allensdk/brain_observatory/ecephys/nwb/AIBS_ecephys_namespace.yaml

  Lines 1 to 6 in a040661

  	namespaces:
  	- doc: ""
  	name: AIBS_ecephys
  	schema:
  	- namespace: core
  	- source: AIBS_ecephys_extension.yaml

  
  You extension does not have a doc, an author, an author email, or a version number. Without a version number, it can be difficult to make adjustments to your extension and retain the ability to validate an NWB file, use visualization and query tools based on your extension, and open your extension across programming languages.

• https://github.com/AllenInstitute/AllenSDK/blob/internal/allensdk/brain_observatory/ecephys/nwb/__init__.py
  In your __init__.py, you define custom API classes for the extensions you have created, but it's not clear why. It looks like you would have gotten the same result by replacing this entire file with

import os

from pynwb import load_namespaces, get_class
import numpy as np


namespace_path = os.path.abspath(os.path.join(os.path.dirname(__file__), "AIBS_ecephys_namespace.yaml"))
pynwb.load_namespaces(namespace_path)

EcephysProbe = get_class('EcephysProbe', 'AIBS_ecephys')
EcephysLabMetaData = get_class('EcephysLabMetaData', 'AIBS_ecephys')

def to_dict(self):
        out = {}
        for key in self.__nwbfields__:
            if key not in ("name", "help"):
                out[key] = getattr(self, key)
        return out

EcephysLabMetaData.to_dict = to_dict

• https://github.com/AllenInstitute/AllenSDK/blob/internal/allensdk/brain_observatory/ecephys/write_nwb/__main__.py#L552
  We advise that you use lfp_writer.write(nwbfile, cache_spec=True). This caches the extension in the NWB file and makes it easier for others to read your data (without having to import allensdk.brain_observatory.ecephys.nwb)

[bendichter]

fyi @rly

[bendichter]

@isaak-willett were all of these points addressed?

[bendichter]

@isaak-willett @kschelonka can we please re-open this?

[kschelonka]

Yes, although we may want to separate some of these points into different issues.

[njmei]

Closing this issue in favor of the children issues that were created.