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

accessing spike waveforms from units table #248

Closed
bendichter opened this issue Feb 15, 2019 · 12 comments · Fixed by #448
Closed

accessing spike waveforms from units table #248

bendichter opened this issue Feb 15, 2019 · 12 comments · Fixed by #448
Assignees

Comments

@bendichter
Copy link
Contributor

We need a standard way to store spike waveforms in a way that they are linked with spike times and to their unit identity. Since we are moving to deprecate Clustering, we can't rely on using a shared index between Clustering and SpikeEventSeries, and we will need to find another way to offer this capability.

One solution might be to add a column to Units that is called 'waveforms' and every cell is a number_of_spikes x number_of_electrode x number_of_samples matrix. An issue with this approach, however, is that we would not be able to store unsorted spike waveforms by the same mechanism. We could also allow SpikeEventSeries specifically for unsorted spikes, but then we have two different ways of storing spike waveforms, and we are trying to avoid that.

Another approach would be to have the 'waveforms' column provide references to rows of the SpikeEventSeries dataset. An advantage of this is that we would have the spikes only in a single place. One disadvantage is that it might be tricky to work out the electrodes issue. Is there a single SpikeEventSeries or multiple, one for each electrode group. If there are multiple, we will need to work out a way for the column to reference different objects between rows. If there is only one, most of the channels will contain zeroes, since spikes are only picked up on a single channel group at a time, and there will be a mismatch between the 'electrodes' column and the dimensions of the waveform.

@ajtritt
Copy link
Member

ajtritt commented Feb 15, 2019

An issue with this approach, however, is that we would not be able to store unsorted spike waveforms by the same mechanism. We could also allow SpikeEventSeries specifically for unsorted spikes, but then we have two different ways of storing spike waveforms, and we are trying to avoid that

I don't think this presents two different ways of storing the same information, unless you equate sorted spikes and unsorted spikes. The purpose of SpikeEventSeries has always been for unsorted spikes. Units by definition is sorted, so storing sorted spike waveforms here fits.

Adding this to Units just means adding two more optional datasets: waveforms (a VectorData and waveforms_index (a VectorIndex).

@bendichter
Copy link
Contributor Author

OK I'm happy with that. Do you anticipate any issues with having a 2D ragged array? (one dim varies, the other is constant length)

@oruebel
Copy link
Contributor

oruebel commented Feb 15, 2019

Just for my own understanding, the waveform column in Units would be in addition to the waveform_mean that is currently in Units?

@ajtritt
Copy link
Member

ajtritt commented Feb 15, 2019

in addition to waveform_mean

@ajtritt
Copy link
Member

ajtritt commented Feb 15, 2019

  • Add links to SpikeEventSeries in ElectrodeGroup
  • create UnitSeries, with link to related SpikeEventSeries
    • include reference to Units table
  • Add link to related UnitSeries to SpikeEventSeries

@bendichter bendichter self-assigned this Feb 15, 2019
@bendichter
Copy link
Contributor Author

Just to bring this convo up to speed. We decided to solve this problem by using the electrode_group column of the Units table. The reference flow will be:
unit -> electrode_group -(link)-> SpikeEventSeries -(link)-> UnitSeries

@bendichter
Copy link
Contributor Author

using #245

@tjd2002
Copy link
Contributor

tjd2002 commented Feb 15, 2019

Sounding good...

Does UnitSeries inherit from SpikeEventSeries? Or does it contain indexes into the related SpikeEventSeries?

Err, let me go read the PR at #245 and try to understand this myself.

@tjd2002
Copy link
Contributor

tjd2002 commented Feb 15, 2019

So, as I understand it, in the latest proposal, we are not adding any new columns to the Units table.

Instead, if you wanted to recover the waveforms for an individual unit, you would follow the reference flow @bendichter outlined above ( unit -> electrode_group -(link)-> SpikeEventSeries -(link)-> UnitSeries ). Then you would parse the data column of the UnitSeries to find the indexes (into the SpikeEventSeries) for a given unit, and go look up the waveform data from the SpikeEventSeries.

In general I like this: I think it's good to have all waveforms stored in one place (using an existing NWB data structure!), and to index into it with a single UnitSeries.

However, it is currently possible to have more than one pynwb.misc.Units table in an NWB file (e.g. to store multiple different clusterings of the same data), and I'm not sure how this is proposed to be handled. That is, in this scheme, how can a given Units table indicate which UnitSeries should be used? By the time you've followed the links through electrode_group and SpikeEventSeries you've lost that information).

I note that the UnitSeries proposed in #245 does have a link to its corresponding Units table, so I suppose you could grab all the related UnitSeries from the SpikeEventSeries and look for a match to the Units table being inspected? (This would only work in the case where there is at most 1 UnitSeries per Units table (which I think sounds reasonable, but should probably be formally stated somewhere)).

@bendichter
Copy link
Contributor Author

bendichter commented Feb 15, 2019 via email

@bendichter
Copy link
Contributor Author

bendichter commented Feb 16, 2019

SpikeEventSeries currently takes an argument electrodes that is a DynamicTableRegion of the electrodes table (via ElectrodeSeries inheritance). Since we are proposing to link SpikeEventSeries to ElectrodeGroups, does it make sense to remove this? This would be a breaking change. We could make it optional and deprecated.

@bendichter
Copy link
Contributor Author

@rly and I discussed a much simpler solution, which would be to have an optional "waveforms" column on the Units table. This would be for storing the waveforms of units that have already been sorted. SpikeEventSeries would continue to be used for storing waveforms of unsorted spikes.

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 a pull request may close this issue.

4 participants