Created new ndx-photostim extension

[lafosse]

nwb extension for storing experiment metadata related to holographic photostimulation methods

[rly]

Thanks for your submission to the NDX Catalog!

The extension looks good.

I reviewed your spec as well and noticed a pattern that is technically allowed but goes against our best practices. In create_extension_spec.py, spec definitions are copied into multiple specs. This syntax may feel natural but results in a bloated spec file and could result in unexpected behavior in the APIs when resolving which spec to use for a given neurodata type.

To be more specific, in create_extension_spec.py, you define new neurodata types SpatialLightModulator and Laser as so:

    slm = NWBGroupSpec(
        neurodata_type_def='SpatialLightModulator',
        neurodata_type_inc='Device',
        name='slm',
        ...
    )

    lsr = NWBGroupSpec(
        neurodata_type_def='Laser',
        neurodata_type_inc='Device',
        name='laser',
        ...
    )

This is good. However, you define new neurodata type PhotostimulationMethod as so:

    psm = NWBGroupSpec(
        neurodata_type_def='PhotostimulationMethod',
        neurodata_type_inc='NWBContainer',
        doc=("Methods used to apply patterned photostimulation."),
        name='method',
        ...
        groups=[
            slm,
            lsr
        ]
    )

which will copy and nest the spec definitions for SpatialLightModulator and Laser into the definition for PhotostimulationMethod. You can see this in the generated YAML files. While technically allowed, this pattern can result in unexpected behavior in the APIs. (We have some ideas on how to improve the user experience to help users follow the better pattern, but these have not yet been implemented).

Instead of the above pattern, I suggest defining PhotostimulationMethod like so:

    psm = NWBGroupSpec(
        neurodata_type_def='PhotostimulationMethod',
        neurodata_type_inc='NWBContainer',
        doc=("Methods used to apply patterned photostimulation."),
        name='method',
        ...
        groups=[
            NWBGroupSpec(
				neurodata_type_inc='SpatialLightModulator',
		        name='slm',
				doc=("Spatial light modulator (SLM) used in the experiment."),
			),
			NWBGroupSpec(
				neurodata_type_inc='Laser',
		        name='laser',
				doc=("Laser used in the experiment."),
			),
        ]
    )

The Python analog for this situation is that in your current code you define three classes, but in the PhotostimulationMethod class you copy and nest the other two classes:

class SpatialLightModulator:
    ...
class Laser:
    ...
class PhotostimulationMethod:
	class SpatialLightModulator:
    	...
	class Laser:
	    ...

whereas the better pattern is to have PhotostimulationMethod include an instance of the other classes:

class SpatialLightModulator:
    ...
class Laser:
    ...
class PhotostimulationMethod:
	def __init__(slm: SpatialLightModulator, laser: Laser):
		self.slm = slm
		self.laser = laser

This pattern should be more reliable, especially in Matlab uses where the classes are all auto-generated from the schema and expect particular schema patterns.

This also occurs in your extension where HolographicPattern copies and nests the definition for PhotostimulationMethod, and PhotostimulationSeries copies and nests the definition for HolographicPattern.

I hope that makes sense, and sorry that this is not well-documented. If you would like, I could make a PR on your extension with the suggested changes, or you could implement them. Let me know!

[lafosse]

Hi Ryan, thanks for the feedback. We can work to implement these changes on our side.

Also, we recently heard there was an effort by CatalystNeuro to create a similar extension for holographic photostimulation data (see catalystneuro/ndx-holographic-stimulation#6).

Perhaps to clarify, is there intent to publish our module after these requested changes? I'm wondering if it would be best to discuss with CatalystNeuro about design more before moving forward here.

Also cc'ing @histed @ryovanno

[rly]

We can publish this extension as is now or after you make the requested changes. I think it would be better and easier to make the changes and then publish. Otherwise, you would need to update the catalog entry to point to the new release. Either way is fine - let me know what you prefer.

The NDX Catalog can contain multiple extensions that serve a similar purpose. However, if a developer wants to create a new extension, we encourage them to browse the catalog for a similar one first, collaborate with its developer, and build on that before creating a new one.

In this case, I understand that both groups developed a similar extension independently and might want to combine efforts on a single extension. You could both publish your extensions now, then collaborate on one, publish/update the co-designed one, and deprecate the other. Or you could publish one or neither of yours now and then collaborate. That is up to you and whether you think your extensions would be useful for others to see now rather than later. An advantage of publishing now is that others can see that you have built this extension and can reach out before starting a similar effort.

[CodyCBakerPhD]

@lafosse @histed @ryovanno

Hey all, it was great to find others interested in making an extension to support this awesome new type of neurodata!

Perhaps to clarify, is there intent to publish our module after these requested changes? I'm wondering if it would be best to discuss with CatalystNeuro about design more before moving forward here.

Here is what I would propose; it comes down to the timescale your group needs for your current purposes

If you already have NWB files that you work with internally or perhaps an entire dataset that you would like to share soon on DANDI, then it might make the most sense for you to use your ndx-photostim for that purpose

If you have some time to wait before using either extension however, I think it would be best to join forces in a way that would result in only a single obvious extension for a new user to find listed on the catalogue

This would also be a path forward to forming a working group to submit an official NEP to get the final result of this work integrated into the core NWB standard

As an aside - this reminds me of an occasion from the past; development of the ndx-pose extension, which worked on both the DLC and SLEAP formats first and which was later found and used for LightningPose (and soon HRNet). This approach felt more effective as a strategy and resulted in a more richly detailed extension that was beneficial for all groups involved, and avoided confusion that may have resulted from there being multiple different (yet similar) extensions for each style

Anyway, I'd like to hear what timeline you all envision for this and any other thoughts you might have. I'll let @alessandratrapani take lead on organizing a joint venture if that sounds ideal, as they have already put some work into merging metadata structures from ndx-photostim into ndx-holographic-stimulation on this unmerged PR

[lafosse]

Thanks for the input! Our current extension works well for our needs now, so I think it will be useful to implement the changes Ryan originally listed above, and then continue with the merge here.

@rly to confirm, the only change needed is in the create_extension_spec.py file?

After that, I think it makes sense to chat about integration with the sister extension to see what will work best for everyone's needs! We'll be comparing the designs soon to see what the key differences are.

[rly]

@rly to confirm, the only change needed is in the create_extension_spec.py file?

You would need to make changes to src/spec/create_extension_spec.py and then re-run that script to re-generate the spec/ndx-photostim.extensions.yaml file. I believe the rest of the extension should not need to be changed and downstream Python code should not need to change, but please test that.

[lafosse]

Have now made the changes and tested to make sure it all works. Is all good to merge or is anything else needed? Thanks

[rly]

That's all that's needed. Thanks!

I do encourage you to work with @alessandratrapani on a joint extension across labs that we can merge into the core NWB schema for the benefit of the broader community. I believe she has invited Dr. Histed to a meeting to discuss.