-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add SpikeEventSeries data type #92
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks good. The main question I have is how we can prevent duplication of 1) channels in the ElectrodeTable
, 2) ElectrodeGroup
s , 3) Device
s
Co-authored-by: Oliver Ruebel <[email protected]>
For duplication of channels in the
Thoughts on that approach? |
src/nwb/NWBFile.cpp
Outdated
std::string devicePath = "/general/devices/" + groupName; | ||
std::string electrodePath = "/general/extracellular_ephys/" + groupName; | ||
std::string electricalSeriesPath = rootPath + groupName; | ||
std::string sourceName = channelVector[0].sourceName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I realized when adding tests for a file that has both ElectricalSeries
and SpikeEventSeries
was that we needed to have a way to specify that data came from the same source (e.g. the same device) but had different names within the acquisition group. I added this sourceName
as a separate input to the Channel
class as a way to accommodate this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is sourceName
the same as the name of the device
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now updated so that groupName
is used for the ElectrodeGroup and Device and is pulled from the channelVector
input. The name of the dataset within the acquisition
group is set by a recordingArrayNames
input to createElectricalSeries
and createSpikeEventSeries
.
Added tests and now also have it integrated with OpenEphys - see here for the latest version of that branch. |
Seems reasonable. I think 2. assumes that there are no custom columns added to the table. We may want to at least check that we have data for all columns |
@oruebel this is ready for you to review again. I've created issues for what we want to address outside this PR |
Will fix #30 |
// Setup electrodes and devices | ||
std::string groupName = channelVector[0].groupName; | ||
std::string devicePath = "/general/devices/" + groupName; | ||
std::string electrodePath = "/general/extracellular_ephys/" + groupName; | ||
std::string electricalSeriesPath = rootPath + groupName; | ||
std::string electricalSeriesPath = acquisitionPath + "/" + recordingName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now this seems fine. I assume this is part of #93 to address the groupName?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By address the groupName
do you mean allowing different options for Device
and ElectrodeGroup
?
From what I had in my notes we also discussed having a single groupName
for a channelVector
. That would be part of #37
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
These changes add the
SpikeEventSeries
data type for recording threshold crossings during data acquisition. In addition to adding the new data type, these changes check whether theElectrodesTable
already exists on theNWBFile
. (Note: what I've written so far assumes that once the electrodes table has been created, all electrodes have been added. I think theElectrodesTable
/DynamicTable
classes would require further changes if we want to append to an existing table).I think some additional tests are needed to check that the code :