-
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
[Proposal] The return of ElectrodesTable #3
Comments
Can you draw it up in mermaid really quick? What is the different between your 'Electrode' and a 'Contact'? A separate object for physical space seems fine, but how optional will it be in the link for the final 'ElectricalSeries' ( |
So, in your proposal how would the ElectrodesTable would be linked or related to the ChannelsTable to avoid this problem. This is not clear to me. I have another proposal to solve the data redundancy: In the current electrodes table each row can have an associated electrode_group, right? Could we use the same mechanism to make the link to probe and insertion in the ChannelsTable at the row level instead of an attribute as it is now? With this, we can link a set of channels to the same Probe for each of the two ChanenlsTable in the neuropixels case. This will avoid the redundancy of having two probes and two probe insertions in the nwbfile for this case without creating more objects. Plus, I am dealing with a conversion right now where an ExtracellularSeries comes from two probes. This is not covered with the current model but could be easily fixed with my proposal above. |
From #1 (Updated 2024-07-22): %%{init: {'theme': 'base', 'themeVariables': {'primaryColor': '#ffffff', "primaryBorderColor': '#144E73', 'lineColor': '#D96F32'}}}%%
classDiagram
direction LR
class ExtracellularSeries {
<<TimeSeries>>
data : numeric
--> unit : str = "microvolts"
channels : DynamicTableRegion
--> target : ChannelsTable
channel_conversion : List[float], optional
--> axis : int = 1
}
class ChannelsTable {
<<DynamicTable>>
--------------------------------------
attributes
--------------------------------------
name : str
description : str
probe : Probe
position_reference : str, optional
electrical_reference_description : str, optional
ground : str, optional
position_confirmation_method : str, optional
--------------------------------------
columns
--------------------------------------
id : VectorData[int]
contact : DynamicTableRegion
--> target : ContactsTable
reference_contact : DynamicTableRegion, optional
--> target : ContactsTable
filter : VectorData[str], optional
---> Strings such as "Bandpass 0-300 Hz".
estimated_position_ap_in_mm : VectorData[float], optional
estimated_position_ml_in_mm : VectorData[float], optional
estimated_position_dv_in_mm : VectorData[float], optional
estimated_brain_area : VectorData[str], optional
confirmed_position_ap_in_mm : VectorData[float], optional
confirmed_position_ml_in_mm : VectorData[float], optional
confirmed_position_dv_in_mm : VectorData[float], optional
confirmed_brain_area : VectorData[str], optional
... Any other custom columns, e.g., ADC information
}
class ProbeInsertion {
<<Container>>
insertion_position_ap_in_mm : float, optional
insertion_position_ml_in_mm : float, optional
insertion_position_dv_in_mm : float, optional
position_reference : str, optional
hemisphere : Literal["left", "right"], optional
insertion_angle_pitch_in_deg : float, optional
insertion_angle_roll_in_deg : float, optional
insertion_angle_yaw_in_deg : float, optional
depth_in_um : float, optional
}
namespace ProbeInterface {
class Probe {
}
class ProbeModel {
}
class ContactsTable {
}
}
Probe *--> ProbeInsertion: might contain ProbeInsertion
ExtracellularSeries ..> ChannelsTable : links to channels
ChannelsTable *..> Probe : links to probe
ChannelsTable ..> ContactsTable : row reference to contact
note for ChannelsTable "ChannelsTable is no longer global"
This results in: %%{init: {'theme': 'base', 'themeVariables': {'primaryColor': '#ffffff', "primaryBorderColor': '#144E73', 'lineColor': '#D96F32'}}}%%
classDiagram
class ExtracellularSeriesAP {
<<ExtracellularSeries>>
}
class ExtracellularSeriesLF {
<<ExtracellularSeries>>
}
class ChannelsTableAP {
<<ChannelsTable>>
}
class ChannelsTableLF {
<<ChannelsTable>>
}
class ProbeInsertion {
<<ProbeInsertion>>
}
class ContactsTable {
<<ContactsTable>>
}
ExtracellularSeriesAP ..> ChannelsTableAP : links to channels
ExtracellularSeriesLF ..> ChannelsTableLF : links to channels
ChannelsTableAP *..> Probe : links to probe
ChannelsTableLF *..> Probe : links to probe
ChannelsTableAP ..> ContactsTable : row reference to contact
ChannelsTableLF ..> ContactsTable : row reference to contact
Probe *--> ProbeInsertion: might contain ProbeInsertion
Proposed (add ElectrodesTable and AnatomicalCoordinatesTable): %%{init: {'theme': 'base', 'themeVariables': {'primaryColor': '#ffffff', "primaryBorderColor': '#144E73', 'lineColor': '#D96F32'}}}%%
classDiagram
direction LR
class ExtracellularSeries {
<<TimeSeries>>
data : numeric
--> unit : str = "microvolts"
channels : DynamicTableRegion
--> target : ChannelsTable
channel_conversion : List[float], optional
--> axis : int = 1
}
class ChannelsTable {
<<DynamicTable>>
--------------------------------------
attributes
--------------------------------------
name : str
description : str
reference_mode : str, optional
--------------------------------------
columns
--------------------------------------
id : VectorData[int]
electrode : DynamicTableRegion
--> target : ElectrodesTable
reference_electrode : DynamicTableRegion, optional
--> target : ElectrodesTable
filter : VectorData[str], optional
---> Strings such as "Bandpass 0-300 Hz".
... Any other custom columns, e.g., ADC information
}
class ElectrodesTable {
<<DynamicTable>>
--------------------------------------
attributes
--------------------------------------
name : str
description : str
probe : Probe
probe_insertion : ProbeInsertion, optional
--------------------------------------
columns
--------------------------------------
id : VectorData[int]
contact : DynamicTableRegion
--> target : ContactsTable
estimated_brain_area : VectorData[str], optional
... Any other custom columns about physical electrode
}
class ProbeInsertion {
<<Container>>
insertion_position_ap_in_mm : float, optional
insertion_position_ml_in_mm : float, optional
insertion_position_dv_in_mm : float, optional
position_reference : str, optional
hemisphere : Literal["left", "right"], optional
insertion_angle_pitch_in_deg : float, optional
insertion_angle_roll_in_deg : float, optional
insertion_angle_yaw_in_deg : float, optional
depth_in_um : float, optional
}
class AnatomicalCoordinatesTable {
<<DynamicTable>>
--------------------------------------
attributes
--------------------------------------
name : str
description : str
space : Space - not shown, defines x, y, z reference space
method : str - e.g., "reconstruction from histology using SHARP-Track" or "estimated"
--------------------------------------
columns
--------------------------------------
id : VectorData[int]
x : VectorData[float]
y : VectorData[float]
z : VectorData[float]
brain_area : VectorData[str]
target_object : DynamicTableRegion
--> target : ElectrodesTable or FibersTable
}
namespace ProbeInterface {
class Probe {
}
class ProbeModel {
}
class ContactsTable {
}
}
ExtracellularSeries ..> ChannelsTable : links to channels
ElectrodesTable *..> Probe : links to probe
ChannelsTable ..> ElectrodesTable : row reference to electrode
ElectrodesTable ..> ContactsTable : row reference to contact
Probe *--> ProbeInsertion: might contain ProbeInsertion
note for ChannelsTable "ChannelsTable is no longer global"
note for ElectrodesTable "ElectrodesTable is no longer global"
AnatomicalCoordinatesTable ..> ElectrodesTable : row reference to electrode
This results in: %%{init: {'theme': 'base', 'themeVariables': {'primaryColor': '#ffffff', "primaryBorderColor': '#144E73', 'lineColor': '#D96F32'}}}%%
classDiagram
class ExtracellularSeriesAP {
<<ExtracellularSeries>>
}
class ExtracellularSeriesLF {
<<ExtracellularSeries>>
}
class ChannelsTableAP {
<<ChannelsTable>>
}
class ChannelsTableLF {
<<ChannelsTable>>
}
class ElectrodesTable {
<<ElectrodesTable>>
}
class ProbeInsertion {
<<ProbeInsertion>>
}
class ContactsTable {
<<ContactsTable>>
}
class EstimatedAnatomicalCoordinatesTable {
<<AnatomicalCoordinatesTable>>
}
class ConfirmedAnatomicalCoordinatesTable {
<<AnatomicalCoordinatesTable>>
}
ExtracellularSeriesAP ..> ChannelsTableAP : links to channels
ExtracellularSeriesLF ..> ChannelsTableLF : links to channels
ElectrodesTable *..> Probe : links to probe
ChannelsTableAP ..> ElectrodesTable : row reference to electrode
ChannelsTableLF ..> ElectrodesTable : row reference to electrode
ElectrodesTable ..> ContactsTable : row reference to contact
Probe *--> ProbeInsertion: might contain ProbeInsertion
EstimatedAnatomicalCoordinatesTable ..> ElectrodesTable : row reference to electrode
ConfirmedAnatomicalCoordinatesTable ..> ElectrodesTable : row reference to electrode
|
@h-mayorquin The ElectrodesTable would have a column "contact". Each row of "contact" would be a reference to a row of the ContactsTable.
In the current proposal, there would be one Probe and one ContactsTable, so the row-wise reference to Probe is not necessary. The ChannelsTable could contain an attribute (or row-wise reference) to the same ProbeInsertion in the same way that it links to the same Probe. That would help. But information about the estimated and confirmed positions and brain areas of each electrode would still be duplicated across the ChannelsTableAP and ChannelsTableLF.
Why not make a separate ExtracellularSeries for each probe? |
So as you mentioned in #1 of the devices tyipically present in an ecephys setup we only represent the probes as nwb devices. The DAQ/Recording and the headstages are lumped into the channels table and we do the wiring through the contact column in #1. I think clumping headstage-daq into the channels table is a good trade-off between simplicity and expresivity. To make this concrete some specific setups are:
Now, why did I mentioned this context? This is to anwser why I would like to keep all the data in the same channels table. So, two reasons:
I am not sure how unique is this case but there are definitley more DAQs that can take more that one probe and will represent data as one stream. I think doing the mappings to the other structures (contacts, probes, andanatomatical tables) per row of the channels table is very neat because it allows us to create simple automated conversions that allow an increasing but not limiting degree of provenance capture if the information is available. In short, if the information is available you map the channels to the information and if not, then you at least can add the channels. More concrently:
With this mapping your scenario only needs one device and my scenario only needs one channel table. Adding anatomical coordinates does not need an extra table because we just indicate a mapping. Maybe there was a good reason we decided against doing a per-row mapping and I do not remember. In that case, please let me know. |
@h-mayorquin If I understand your proposal correctly, each row in the In the original proposal, we would have four Your proposal makes sense. However, in current NWB, data from a DAQ are sometimes organized into different objects to improve interpretability and reuse. For example, position, eye tracking, joystick movements, and other behavioral data are often acquired by the DAQ as analog channels, and these are stored in different NWB neurodata types. I think data from an ECoG grid and a probe recorded simultaneously are usually stored in two separate
I think the references and other channel information will not always be the same across channels, so it would be best to store these on a per-channel basis for maximum expressivity. This will already results in duplication regardless of whether
I agree, but the experimenter should have the information of which channels belonged to which probe, and I think splitting data from the DAQ into separate objects by probe would slightly improve interpretability and downstream processing of the data. A user might want to spike sort all channels of the same probe together and separately from channels from another probe. They can do that under either proposal, but it may be slightly more intuitive under the original proposal.
I do not remember; I think it felt cleaner and as a way of reducing duplication. I do not feel strongly about having each channel in a |
I think we should not allow case 1 and we should require that having an If we make I cannot think of a reason why a user would not have those basic probe metadata, and if we allow people to not specify that, some won't and that would reduce the reusability of the data. |
This is a good point. Some DAQS have data that is definitley not ecephys so the identification is not perfect. What we want to map to the ChannelsTable is a subset of the DAQ channels: the ecephys channels. Thanks for this clarification.
Fair enough, that's probably too permisive. They can use something else for that case. I agree with you.
Probeinterface has the concept of probe group and spikeinterface
@rly , I discussed this today with @CodyCBakerPhD in a personal meeting and he says he is OK with this. Please do confirm when you have a chance @CodyCBakerPhD . If you feel neutral about it I can do this PR.
To be fair, now that we have probe insertion linked to probe and if you agree to make the mapping to the probes per row I don't think I have a strong view on this. I feel that the actual series should be separated (so 4 in this case) but I am less certain about the ChannelsTable. Following my own logic about the correspondence between ChannelsTable and a subset of the DAQ, there should only be one table and all the series should map to different regions of the same table. That is, assuming they were acquired with the PXI-e/Imec card duo as depicted here. But I actually not very sure. I will create a separate issue for the AnatomicalCoordinatesTable because I think this issue already has too many threads and some of them have already been solved but I think that point deserves its own issue. |
It will make more sense when I see it written in Mermaid or schematically on a draft PR, but in in principle if there is a real life use case that facilitates the adjustment then it should be done |
Mmm after working through some bugs with electrode table on neuroconv I have changed my mind about row mapping to device fot fhe following reasons:
All of those seems like good reasons to keep the current correspondece of one |
Hi, @rly , sorry for kind of deraling the intent of your initial comment. I thought we got caught up between discussing the neuropixel case that you bought up, the I re-open another issue to center the discussion again along the lines of your original intent #8 . Maybe we can close this one? |
Problem: Neuropixels 1.0 probes write data from the same physical electrode contact to two different streams, AP and LF [ref].
In the latest iteration #1 , a Neuropixels 1.0 recording would consist of:
ExtracellularSeries
object for AP dataChannelsTable
that contains only the AP channelsChannelsTable
references a row of theContactsTable
associated with the Neuropixels 1.0ProbeModel
ExtracellularSeries
object for LF dataChannelsTable
that contains only the LF channelsChannelsTable
references a row of theContactsTable
associated with the Neuropixels 1.0ProbeModel
If the user wants to specify information about the probe insertion, then they have to create two identical
ProbeInsertion
objects, one for eachChannelsTable
. If the user wants to specify information about the estimated and confirmed positions of each contact, they have to specify it in bothChannelsTable
objects.Alternatively, we could pull out these physical properties of the inserted probe into an
ElectrodesTable
. ThisElectrodesTable
would contain:ProbeInsertion
tableestimated_brain_area
This
ElectrodesTable
would contain information about the physical electrode contact. TheChannelsTable
would contain information about virtual channels, or data streams, that may have different properties (e.g., hardware filtering, gain, maybe referencing).@bendichter @stephprince and I are also proposing a separate object (
CoordinatesTable
?) to store the estimated localization and confirmed localization of electrodes in some reference space and atlas. It would have coordinates, brain area, method, and reference space information. One object for estimated, one object for confirmed using method 1, one object for confirmed using method 2 (in case there are two). EachCoordinatesTable
would point to theElectrodesTable
. (Under the model of #1, eachCoordinatesTable
would have had to point to bothChannelsTable
objects above or be duplicated, which are not ideal.)Pro: Reduced duplicate data
Con: For people storing non-Neuropixels 1.0 data, this
ElectrodesTable
will feel needlessly complex. We may be able to address that by adding convenience functions in the Python API.@CodyCBakerPhD @h-mayorquin what do you think?
The text was updated successfully, but these errors were encountered: