NeurodataWithoutBorders / nwbwidgets

Explore the hierarchical structure of NWB 2.0 files and visualize data with Jupyter widgets.
https://nwb-widgets.readthedocs.io/en/latest/
Other
48 stars 21 forks source link

Fix fancy indexing for electrical series with Zarr backend #283

Open alejoe91 opened 1 year ago

alejoe91 commented 1 year ago

Turns out fancy indexing is not supported for zarr.Dataset objects and this was causing an issue in displaying timeseries correctly.

This small PR fixes it by changing the behavior of the timeseries.py and:

Not that for NWBwidgets in all cases we can slice directly, because of the channel slider.

codecov[bot] commented 1 year ago

Codecov Report

Attention: Patch coverage is 80.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 66.24%. Comparing base (fdc0309) to head (03a598a).

Files Patch % Lines
nwbwidgets/timeseries.py 80.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #283 +/- ## ======================================= Coverage 66.24% 66.24% ======================================= Files 35 35 Lines 3478 3481 +3 ======================================= + Hits 2304 2306 +2 - Misses 1174 1175 +1 ``` | [Flag](https://app.codecov.io/gh/NeurodataWithoutBorders/nwbwidgets/pull/283/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NeurodataWithoutBorders) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/NeurodataWithoutBorders/nwbwidgets/pull/283/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NeurodataWithoutBorders) | `66.24% <80.00%> (+<0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NeurodataWithoutBorders#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

bendichter commented 1 year ago

@alejoe91 it looks like Zarr does actually support fancy indexing, though the syntax is different from numpy

https://zarr.readthedocs.io/en/stable/tutorial.html#advanced-indexing

alejoe91 commented 1 year ago

@bendichter should we use the Zarr fancy indexing functions when the dataset is a Zarr object then?

bendichter commented 1 year ago

Yes that would be an immediate fix. But this issue is going to come up for anything downstream of pynwb so maybe it makes sense to fix on the pynwb level. Maybe we could wrap all arrays in xarray?

bendichter commented 1 year ago

@alejoe91 can you also please add checks that this data indexing utility does indeed solve the problem for Zarr dataset objects?