Skip to content
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

[Bug]: RoiResponseSeries visualizations do not work on ndx-photometry #284

Closed
2 tasks done
CodyCBakerPhD opened this issue Jun 3, 2023 · 5 comments
Closed
2 tasks done
Assignees
Labels
bug Something isn't working

Comments

@CodyCBakerPhD
Copy link
Collaborator

What happened?

@pauladkisson recently made some nice NWB files of fibre photometry data using the ndx-photometry, but this causes a pretty severe issue with the default traces visualizer for that (which ought to be pretty simple otherwise)

Steps to Reproduce

No response

Traceback

KeyError                                  Traceback (most recent call last)
File /opt/conda/lib/python3.10/site-packages/ipywidgets/widgets/widget.py:766, in Widget._handle_msg(self, msg)
    764         if 'buffer_paths' in data:
    765             _put_buffers(state, data['buffer_paths'], msg['buffers'])
--> 766         self.set_state(state)
    768 # Handle a state request.
    769 elif method == 'request_state':

File /opt/conda/lib/python3.10/site-packages/ipywidgets/widgets/widget.py:643, in Widget.set_state(self, sync_data)
    638         self._send(msg, buffers=echo_buffers)
    640 # The order of these context managers is important. Properties must
    641 # be locked when the hold_trait_notification context manager is
    642 # released and notifications are fired.
--> 643 with self._lock_property(**sync_data), self.hold_trait_notifications():
    644     for name in sync_data:
    645         if name in self.keys:

File /opt/conda/lib/python3.10/contextlib.py:142, in _GeneratorContextManager.__exit__(self, typ, value, traceback)
    140 if typ is None:
    141     try:
--> 142         next(self.gen)
    143     except StopIteration:
    144         return False

File /opt/conda/lib/python3.10/site-packages/traitlets/traitlets.py:1502, in HasTraits.hold_trait_notifications(self)
   1500 for changes in cache.values():
   1501     for change in changes:
-> 1502         self.notify_change(change)

File /opt/conda/lib/python3.10/site-packages/ipywidgets/widgets/widget.py:694, in Widget.notify_change(self, change)
    691     if name in self.keys and self._should_send_property(name, getattr(self, name)):
    692         # Send new state to front-end
    693         self.send_state(key=name)
--> 694 super().notify_change(change)

File /opt/conda/lib/python3.10/site-packages/traitlets/traitlets.py:1517, in HasTraits.notify_change(self, change)
   1515 def notify_change(self, change):
   1516     """Notify observers of a change event"""
-> 1517     return self._notify_observers(change)

File /opt/conda/lib/python3.10/site-packages/traitlets/traitlets.py:1564, in HasTraits._notify_observers(self, event)
   1561 elif isinstance(c, EventHandler) and c.name is not None:
   1562     c = getattr(self, c.name)
-> 1564 c(event)

File /opt/conda/lib/python3.10/site-packages/nwbwidgets/base.py:130, in lazy_tabs.<locals>.on_selected_index(change)
    128 def on_selected_index(change):
    129     if isinstance(change.owner.children[change.new], widgets.HTML):
--> 130         children[change.new] = vis2widget(tabs_spec[change.new][1](node))
    131         change.owner.children = children

File /opt/conda/lib/python3.10/site-packages/nwbwidgets/ophys.py:325, in RoiResponseSeriesWidget.__init__(self, roi_response_series, neurodata_vis_spec, **kwargs)
    324 def __init__(self, roi_response_series: RoiResponseSeries, neurodata_vis_spec=None, **kwargs):
--> 325     super().__init__(roi_response_series, "rois", **kwargs)

File /opt/conda/lib/python3.10/site-packages/nwbwidgets/timeseries.py:594, in BaseGroupedTraceWidget.__init__(self, time_series, dynamic_table_region_name, foreign_time_window_controller, foreign_group_and_sort_controller, mpl_plotter, **kwargs)
    592     table = dynamic_table_region.table
    593     referenced_rows = np.array(dynamic_table_region.data)
--> 594     self.gas = GroupAndSortController(
    595         dynamic_table=table,
    596         keep_rows=referenced_rows,
    597     )
    598     self.controls.update(gas=self.gas)
    599 else:

File /opt/conda/lib/python3.10/site-packages/nwbwidgets/controllers/group_and_sort_controllers.py:140, in GroupAndSortController.__init__(self, dynamic_table, group_by, window, keep_rows, control_order, control_limit, groups)
    133 range_controller_max = min(30, self.nitems)
    134 dt_desc_map = {
    135     "DynamicTable": "traces",
    136     "TimeIntervals": "trials",
    137     "Units": "units",
    138     "PlaneSegmentation": "image_planes",
    139 }
--> 140 desc = dt_desc_map[dynamic_table.neurodata_type] if dynamic_table is not None else "traces"
    141 self.range_controller = RangeController(
    142     0,
    143     self.nitems,
   (...)
    147     orientation="vertical",
    148 )
    149 self.range_controller.layout = Layout(min_width="75px")

KeyError: 'FibersTable'

Operating System

Linux

Python Version

3.10

Package Versions

The default ipykernel on the DANDI hub

Code of Conduct

@CodyCBakerPhD CodyCBakerPhD added the bug Something isn't working label Jun 3, 2023
@pauladkisson
Copy link

Thanks @CodyCBakerPhD, I will check it out.

@pauladkisson
Copy link

Hey @CodyCBakerPhD,

I don't totally understand this issue.

The fiber photometry data uses the ndx-photometry extension, so it has neurodata_type = 'FiberTable', which obviously isn't one of the neurodata_types supported by nwbwidgets.

I would expect there to be some way to extend the nwbwidgets so that they support some specific nwb extention, but I don't how to do that, and the description on the docs doesn't really provide enough info to integrate with the nwb2widget() function.

@CodyCBakerPhD
Copy link
Collaborator Author

The question is, why does the RoiResponseWidget require the FibersTable at all?

There are already some extension data types (like grayscale volume) included in the core widgets; you could always do a dynamic non-required update to the neurodata_viz_spec with a couple lines that detect if ndx-photometry is installed in the current environment

Or, setup a fallback to a basic TimeSeries visualization in this situation - there's lots of possibilities for improving it - but point being it's not a great look if we try to visualize these files with the widgets and they fail to render the main neural data in the file

@pauladkisson
Copy link

Update:
I was able to get the o-phys data to visualize by "FibersTable" : "traces" to the dt_desc_map dictionary in group_and_sort_controllers.py (I also had to omit yticks/yticklabels in timeseries.py for some reason). But, passing a custom function via neurodata_vis_spec doesn't seem to be doing anything.

Rather than quickly pushing a hacky fix to nwbwidgets, I'd like to take some serious time to understand how all this stuff is working and create nice custom visualizations for the Datta Lab dataset. But, I think fleshing out the conversion (optogenetic datainterface, dandiset, etc.) is much more important right now.

So, my pitch is to leave this issue for now, and swing back around to it when we're ready to really focus on visualizations.

@CodyCBakerPhD
Copy link
Collaborator Author

Replaced by #293

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants