Create extension spec

[alessandratrapani]

See #2
The current implementation follow this schema:

TODOs:

• add commanded voltages
• add tests

[alessandratrapani]

@luiztauffer @pauladkisson @weiglszonja
I forgot to ask you about the commanded voltage series: where would you add them?

[luiztauffer]

I'm not sure I like xxxx_xxxxx_in_nm. I can see some advantage in being explicit about the units, but that's not pynwb standard, usually this is something we define in the doc of the attributes

[luiztauffer]

@alessandratrapani we're also missing the Fluorophore class here, right?

[luiztauffer]

Suggested change

	name="fiber_photometry_table",
	name="fiber_photometry_table_region",

[pauladkisson]

I'm not sure I like xxxx_xxxxx_in_nm. I can see some advantage in being explicit about the units, but that's not pynwb standard, usually this is something we define in the doc of the attributes

I second this.

[pauladkisson]

we're also missing the Fluorophore class here, right?

I think we had decided to just include the fluorophore as a text field (here indicator) to avoid the complexity of excitation/emission spectra (which can be easily looked up by name anyway). We do still need optional injection_location and injection_coordinates fields tho.

PS sorry for editing your comment -- it was an accident.

[pauladkisson]

I forgot to ask you about the commanded voltage series: where would you add them?

How about we have a commanded_voltages field in the FiberPhotometryResponseSeries that points to a CommandedVoltageSeries object with data [ntimes x nfibers] (same shape as FiberPhotometryResponseSeries object)?

[alessandratrapani]

I'm not sure I like xxxx_xxxxx_in_nm. I can see some advantage in being explicit about the units, but that's not pynwb standard, usually this is something we define in the doc of the attributes.

I second this.

We discuss this with @rly and @CodyCBakerPhD. If we store it as an attribute, we add the unit of measures as in xxxx_xxxxx_in_xx. If we store it as a dataset, we can add a unit attribute to it. But they prefer to have it in the name of the field to reinforce the use of the correct U.M.

[alessandratrapani]

we're also missing the Fluorophore class here, right?

I think we had decided to just include the fluorophore as a text field (here indicator) to avoid the complexity of excitation/emission spectra (which can be easily looked up by name anyway). We do still need optional injection_location and injection_coordinates fields tho.

How do you feel about making it an extension of LabMetadata, and adding optional injection_location and injection_coordinates fields?

[luiztauffer]

We discuss this with @rly and @CodyCBakerPhD. If we store it as an attribute, we add the unit of measures as in xxxx_xxxxx_in_xx. If we store it as a dataset, we can add a unit attribute to it. But they prefer to have it in the name of the field to reinforce the use of the correct U.M.

I find this strage still... can you point out any pynwb class that follows this pattern, as an example? And then, if we do this, shouldn't we do this for every attribute that has U.M.? Coordinates, for example...

we're also missing the Fluorophore class here, right?

I think we had decided to just include the fluorophore as a text field (here indicator) to avoid the complexity of excitation/emission spectra (which can be easily looked up by name anyway). We do still need optional injection_location and injection_coordinates fields tho.

How do you feel about making it an extension of LabMetadata, and adding optional injection_location and injection_coordinates fields?

I'm also confused about this, I might have missed the point in our meeting. It feels like indicator (previously fluorophore) has multiple attributes: name, injection_location, injection_coordinates, and potentially others such as description... then wouldn't make sense to have it as a separate class as well? Or, what would be the disadvantage of that?
I'm not sure if that fits well within a LabMetadata... what if there are multiple indicators?
We can, of course, avoid adding excitation/emission wavelengths to avoid the confusion about those attributes.

[rly]

Hi all, regarding units, I recommend we discuss that here: NeurodataWithoutBorders/nwb-schema#569

In brief, I think there are many good reasons to adopt a new convention name_in_units. For backwards compatibility reasons, we cannot easily change the names in the core standard (it is doable but it will take some time to do right). I think that maintaining consistency with a suboptimal convention should not prevent us from improving the standard for new data types. If we agree with the new convention, then I think we should recommend this new convention as a best practice for extensions and in new data types that we add to the core, and consider changing the names for existing fields in the core standard later.

[luiztauffer]

Hi all, regarding units, I recommend we discuss that here: NeurodataWithoutBorders/nwb-schema#569

In brief, I think there are many good reasons to adopt a new convention name_in_units. For backwards compatibility reasons, we cannot easily change the names in the core standard (it is doable but it will take some time to do right). I think that maintaining consistency with a suboptimal convention should not prevent us from improving the standard for new data types. If we agree with the new convention, then I think we should recommend this new convention as a best practice for extensions and in new data types that we add to the core, and consider changing the names for existing fields in the core standard later.

ok cool, if this is the convention that should be followed by any new extensions, I support it!

[pauladkisson]

Hi all, regarding units, I recommend we discuss that here: NeurodataWithoutBorders/nwb-schema#569

I was unaware of this new best practice, but it sounds good to me. Thanks for putting together a clear write-up!

[pauladkisson]

I'm not sure if that fits well within a LabMetadata... what if there are multiple indicators?

I also think LabMetadata is a bit of a weird fit. Indicators are functionally similar to Device (even though calling it a device is a bit of a misnomer) -- they often have manufacturers and lot # identifiers.

[weiglszonja]

Thank you @alessandratrapani for working on this! I'll have to figure out how to modify the existing interfaces for this metadata structure (see my comment above), but overall this looks really great.

[pauladkisson]

Thanks for putting this together @alessandratrapani!

I caught some typos and suggested that we drop MultiCommandedVoltage, but otherwise this is looking great!

[pauladkisson]

LGTM