Add initial extension

[rly]

• Create new extension from latest ndx-template
• Add new spec types in create_extension_spec.py
• Generate specs YAML
• Test auto-generation of classes from schema using get_class
• Add unit tests
• Add integration tests
• Expand example
• Test example code
• Resolve [Bug]: Compound dtype attributes are not supported hdmf-dev/hdmf#1099 before adding ProbeInsertion.insertion_position_in_mm and ProbeInsertion.insertion_angle_in_deg
• Resolve relevant TODOs in pyproject.yaml
• Resolve relevant TODOs in create_extension_spec.py
• Update mermaid UML
• Improve docstrings
• Adapt custom API code from https://github.com/SpikeInterface/ndx-probeinterface/tree/main/src/pynwb

[CodyCBakerPhD]

Wow, this is really coming together! Just let me know when ready for an initial review - I'm OK with quick merge cycles and subsequent fixes/improvements as well

[rly]

Good point. This PR doesn't need to be perfect and I think it is good enough for initial review. We can tackle the remaining TODO items in separate PRs.

[rly]

Actually before review, I'm going to make a few small organizational changes.

[CodyCBakerPhD]

Actually before review, I'm going to make a few small organizational changes.

Sure, what kind of organization changes were you thinking?

[h-mayorquin]

Let me know when I should take a look into this.

[rly]

OK. Sorry for my delay; I admit, I think I sent that last message on a plane and no longer remember what else I wanted to change. So this is ready for review.

[CodyCBakerPhD]

Just a couple minor points

After that happy to merge this and raise issues as follow-ups if bugs are encountered or changes requested

[h-mayorquin]

Remaining TODOs for discussion.

• ContactsTable.relative_position_in_mm.reference ("Reference point for the relative position coordinates and information about the coordinate system used, e.g., which direction is positive in the x direction (first element), which direction is positive in the y direction (second element), etc.") is not required, but I wonder if it should be for maximal interpretability, especially if a tool builder wants to use these coordinates with the insertion position, angle, and depth of the probe to compute the stereotactic coordinates of each contact.

I am confused about this. The contacts table is just an non-spatial positioned version of geometry / shape of the probe. Why do we have an attribute for the relative_position_in_mm? Can we just have an attribute on the table itself indicating the origin? Would that not suffice?

• Should ChannelsTable.contact (the reference to rows of the ContactsTable) be optional? In other words, should a ContactsTable be required to create an ExtracellularSeries. I think probably. We currently require ElectrodeGroup and Device. I'm just thinking about the number of lines of code this will add to the tutorials...

I think should be optional. @CodyCBakerPhD why do you think it should be required? We are not sure if people will have the wiring map and we don't want them to make up information. The identity map is making up information.

[rly]

Thank you for the detailed review @h-mayorquin ! I have updated the PR based on your suggestions and discussion.

[h-mayorquin]

Ok, I read the round-trip tests and this looks OK to me. More than that, this is looking great. Thanks @rly for moving this forward. I learned a lot from your reviewing your code. Thanks for your patience.

There are things that I still don't like but that can be addressed later:

1. I think probe insertion should go into Probe as discussed. I think we agree on this and will also solve some concerns in [Proposal] The return of ElectrodesTable #3 (comment) without too many changes.
2. Make the contacts optional. The wiring between channels and contacts is not obvious and in cases the users don't have it we should not force them to construct fake information. If the mapping is not available, then is not available and tha's it.
3. I don't really like having the estimated and confirmed positions as canonical attributes in the channels table. I think that the proposal on [Proposal] The return of ElectrodesTable #3 of relegating this to an AnatomicalCoordinatesTable is better for the following reasons. First, separation of concerns between the description of the ecephys setup and experiments of localization. I think is nice to decouple those two things so they can evolve on their own. If the localization experiments improve we then only need the change to AnatomicalCoordinatesTable or link into a better extension without making parts of our schema obsolete. Second, in the case of SpikeGLX where two channels map to one contact we will end up with redundant information. Plus, even within the current schema I don't like the canonization of estimated_{} as a set of single columns. I think that confirmed_{x} is a great name but what if there is more than one estimate? Right now those are optional attributes and maybe something like this is needed urgently in which case is fine but I still wanted to point out what I think are flaws of this design.
4. Finally, I would still prefer the mapping between the channels table and device to be at the row level for the reasons described in [Proposal] The return of ElectrodesTable #3 (comment). Which in short are that the channels table is working as an abstraction of the DAQ (recording system plus headstage in most cases) and those devices usually map to more than one probe (like the DAQ of Openephys and Intan). In addition, this will make automatization of the conversion possible for some formats that don't have a way of telling if the data came from differentes probes (like Intan). Plus, mapping at the rows of the channel table to either device or rows of the AnatomicalCoordinatesTable would also solve the problems of duplication of information described on [Proposal] The return of ElectrodesTable #3 (comment) and is a more general workflow overall. I think is a little bit more clumsy but I think some of that can be attenuated with clever design at the API level.

What do you guys think?

[rly]

1. I think probe insertion should go into Probe as discussed. I think we agree on this and will also solve some concerns in [Proposal] The return of ElectrodesTable #3 (comment) without too many changes.

I just moved ProbeInsertion to Probe. We can adjust it again in #3 if we want to.

2. Make the contacts optional. The wiring between channels and contacts is not obvious and in cases the users don't have it we should not force them to construct fake information. If the mapping is not available, then is not available and tha's it.

I'm not sure. ChannelsTable requires Probe, Probe requires ProbeModel, ProbeModel requires ContactsTable, and ContactsTable requires relative_position_in_um. So already the user must know the position of each contact on the probe. I think the user will also know the mapping of their data to contacts on a probe in most cases because that's usually needed to interpret the data. I get that's not always the case. For those who don't, we could propose that they specify an identity mapping and set the relative_position_in_um values to all NaN to indicate they don't know which contact is which.

As I understand, people using tetrodes usually do not know the relative position of individual contacts. Should ContactsTable.relative_position_in_um be optional?

3. I don't really like having the estimated and confirmed positions as canonical attributes in the channels table. I think that the proposal on [Proposal] The return of ElectrodesTable #3 of relegating this to an AnatomicalCoordinatesTable is better for the following reasons. First, separation of concerns between the description of the ecephys setup and experiments of localization. I think is nice to decouple those two things so they can evolve on their own. If the localization experiments improve we then only need the change to AnatomicalCoordinatesTable or link into a better extension without making parts of our schema obsolete. Second, in the case of SpikeGLX where two channels map to one contact we will end up with redundant information. Plus, even within the current schema I don't like the canonization of estimated_{} as a set of single columns. I think that confirmed_{x} is a great name but what if there is more than one estimate? Right now those are optional attributes and maybe something like this is needed urgently in which case is fine but I still wanted to point out what I think are flaws of this design.

Let's discuss this in #3 and move forward here.

4. Finally, I would still prefer the mapping between the channels table and device to be at the row level for the reasons described in [Proposal] The return of ElectrodesTable #3 (comment). Which in short are that the channels table is working as an abstraction of the DAQ (recording system plus headstage in most cases) and those devices usually map to more than one probe (like the DAQ of Openephys and Intan). In addition, this will make automatization of the conversion possible for some formats that don't have a way of telling if the data came from differentes probes (like Intan). Plus, mapping at the rows of the channel table to either device or rows of the AnatomicalCoordinatesTable would also solve the problems of duplication of information described on [Proposal] The return of ElectrodesTable #3 (comment) and is a more general workflow overall. I think is a little bit more clumsy but I think some of that can be attenuated with clever design at the API level.

What do you guys think?

Let's discuss this in #3 and move forward here.

Lastly, in reviewing ContactsTable.relative_position_in_um.reference, I think it is not necessary and so I have removed it. Let's discuss the use case for it in #4 with a clear example before adding it in.

Thank you so much @h-mayorquin and @CodyCBakerPhD for the thorough reviews! I just have one remaining question above. I suggest we discuss the rest of the topics as independent enhancements that we address after merging this already large PR.

[CodyCBakerPhD]

@rly Thanks for pushing on this!