Bugfix/chirp sampling ISSUE 546

[MatthewAitken]

Thank you for contributing to the IPFX, your work and time will help to
advance open science!

Overview:

This PR resolves Issue #546 - sweep extraction failing due to sampling rate for 12.5khz chirp samples, and fixes how sweep features are extracted

Stimulus Ontology needs to be updated to include sweep types with names Chirp, ChirpA, ChirpB, ChirpC, and ChirpD

Addresses:

Addresses issue #546

Type of Fix:

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing
  functionality to not work as expected)
• Documentation Change

Solution:

During my investigation, I realized that the function extract_sweep_features in data_set_features.py was not doing what it was intended to be doing. In short, the variable stimulus_name never matched the keys in DETECTION_PARAMETERS, so the default detection parameters were always being used for extract_sweep_features().
As an example, stimulus_name would be "Short Square" for a short square stimulus, but the key in DETECTION_PARAMETERS was "short_square"

(extract_cell_long_square_features(), extract_cell_short_square_features(), and extract_cell_ramp_features() were working as intended.)

The solution to both of these issues was to update DETECTION_PARAMETERS - correcting the key issue for short square sweeps, and adding a new one for Chirp sweeps, which will set the filter_frequency to None.

Changes:

• Updated how stimulus names are mapped to stimulus types (e.g.
  short_square = {
  "Short Square",
  "Short Square Threshold",
  "Short Square - Hold -60mV",
  "Short Square - Hold -70mV",
  "Short Square - Hold -80mV",
  }
• Added Chirp stimulus type
• Added detection parameters for Chirp stimulus

Validation:

Validated that unit tests pass with expected changes to Short Square Sweep Features (since they now use the correct Detection Parameters)
Validated the data in issue #546 (an nwb file with chirp sweeps) successfully passes run_pipeline

Screenshots:

Unit Tests:

Script to reproduce error and fix:

Configuration details:

Checklist

• My code follows
  Allen Institute Contribution Guidelines
• My code is unit tested and does not decrease test coverage
• I have performed a self review of my own code
• My code is well-documented, and the docstrings conform to
  Numpy Standards
• I have updated the documentation of the repository where
  appropriate
• The header on my commit includes the issue number
• My code passes all tests
• I have updated the CHANGELOG.md with the description of changes understandable by end users

Notes:

Use this section to add anything you think worth mentioning to the
reader of the issue

[gouwens]

I think the changes make sense, but I would categorize them as breaking changes for accessing the sweep names via the ontology object (i.e., capitalizing the constant names) and for moving the definitions out of EphysDataSet into StimulusType. That's kind of evidenced by having to make the changes in run_feature_collection.py and run_feature_vector_extraction.py - those are in a sense examples of "user code", even though they are included with the package.

So I think as long as that is made clear, the PR seems good.

[gouwens]

Just to follow up - another option could be to keep both cases and have the definitions in both places for a time and include a deprecation warning.

[tmchartrand]

Good catch on the detection parameters issue!
Regarding this as a fix for the chirp issue, I'm not quite sure we actually want to always set no filtering as a default for chirps. @ru57y34nn do you know if all chirp sweeps are sampled at lower rate? Some of the external data I've used may be different too, I can try to check.

My existing fix for this was to add a check to the filtering function and ignore invalid filtering parameters with a warning (still maybe a good idea for robustness). I'll let others chime in on whether they think fully disabling filters for chirps is a good idea.

Also a few comments below on the refactoring along with this. I think it's probably fine to move the definitions out of EphysDataSet, but not quite sure about the other refactors.

[tmchartrand]

looks good to me, thanks.

[rpmcginty]

lgtm, minor comments