nwb-extensions / ndx-template

A template for creating Neurodata Extensions for the NWB neurophysiology data standard
https://nwb.org/
Other
5 stars 8 forks source link

Demo widget integration, add notebook #77

Closed rly closed 5 months ago

rly commented 10 months ago

Fix #73, fix #52

Widget classes should go in /src/pynwb/ndx-xxxx/widgets Widgets should be added to the new neurodata type Python class in the .widget attribute. See the changes in __init__.py.

This also adds an example notebook that shows the widget in action. To test, check out this branch, run cookiecutter ., open the notebook in jupyter lab, and run it. When the widget appears, click the "acquisition" tab. The custom TetrodeSeriesWidget should appear. It shows the custom trode_id above the normal ElectricalSeriesWidget defined by nwbwidgets.

rly commented 10 months ago

Python 3.12 tests are failing because aiohttp does not yet support Python 3.12.

bendichter commented 10 months ago

This is great! I just want to bring up a few points for discussion.

  1. auto import

Let's design this in a way that widgets can be automatically added from extensions. I'm not a big fan of magic on import, e.g. putting default_neurodata_vis_spec["type"] = widget in the __init__. I take the lesson from seaborn that magic on import is not always ideal. However, I think the next best thing might be something like

def load_extension_widgets_into_spec(namespaces):
    for ns in namespaces:
        default_neurodata_vis_spec.update(import_library(ns).widgets.vis_spec)

This would require something like vis_spec, which is a dictionary mapping from neurodata type to corresponding widget (or to multiple widgets as a dictionary)

Another option is to add something like this to nwb widgets to dynamically find widgets:

if neurodata_type in default_neurodata_vis_spec:
    return default_neurodata_vis_spec[neurodata_type]
elif hasattr(neurodata_type, "widget"):
    return neurodata_type.widget

in which case the current implementation would work. The downside of this second approach is that it would not allow users to customize the spec before running, though I suppose they could do so by changing the widget attr of ndtype directly.

  1. dependencies

nwb widgets has a very extensive and heavy list of dependencies that can make it a pain to install correctly. I would hate to have an extension inherit this mess. I see that you are handling this already by listing the requirement in dev and doing a robust import in __init__ so that nwbwidgets is not required. That's one way to handle it. Another could be to move the Tetrode.widget = TetrodeWidget to within the file /src/pynwb/{{ cookiecutter.py_pkg_name }}/widgets/tetrode_series_widget.py. That way all widget logic is in its own module and can be safely deleted or ignored.

rly commented 10 months ago

However, I think the next best thing might be something like

def load_extension_widgets_into_spec(namespaces):
    for ns in namespaces:
        default_neurodata_vis_spec.update(import_library(ns).widgets.vis_spec)

This would require something like vis_spec, which is a dictionary mapping from neurodata type to corresponding widget (or to multiple widgets as a dictionary)

Are you imagining that the above code lives in nwbwidgets and the user has to explicitly call it? i.e.:

Put in nwbwidgets:

from nwbwidgets import default_neurodata_vis_spec
import importlib

def load_extension_widgets_into_spec(namespaces: list):
    for ns in namespaces:
        default_neurodata_vis_spec.update(importlib.import_module(ns).widgets.vis_spec)

Usage in user code (I put this in the example notebook):

from nwbwidgets import nwb2widget, load_extension_widgets_into_spec
load_extension_widgets_into_spec("ndx_my_namespace")
nwb2widget(nwbfile)

Another option is to add something like this to nwb widgets to dynamically find widgets:

if neurodata_type in default_neurodata_vis_spec:
    return default_neurodata_vis_spec[neurodata_type]
elif hasattr(neurodata_type, "widget"):
    return neurodata_type.widget

in which case the current implementation would work. The downside of this second approach is that it would not allow users to customize the spec before running, though I suppose they could do so by changing the widget attr of ndtype directly.

This is what I was designing for, but either approach is fine for me. I think I like the first approach above better since it is explicit and customizable.

  1. dependencies

nwb widgets has a very extensive and heavy list of dependencies that can make it a pain to install correctly. I would hate to have an extension inherit this mess. I see that you are handling this already by listing the requirement in dev and doing a robust import in __init__ so that nwbwidgets is not required. That's one way to handle it. Another could be to move the Tetrode.widget = TetrodeWidget to within the file /src/pynwb/{{ cookiecutter.py_pkg_name }}/widgets/tetrode_series_widget.py. That way all widget logic is in its own module and can be safely deleted or ignored.

I made those changes. Let me know what you think.

rly commented 5 months ago

Ping @bendichter . What do you think about this updated PR?