Fix fancy indexing for electrical series with Zarr backend #283
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #283 +/- ##
=======================================
Coverage 66.24% 66.24%
=======================================
Files 35 35
Lines 3478 3481 +3
=======================================
+ Hits 2304 2306 +2
- Misses 1174 1175 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@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 |
|
@bendichter should we use the Zarr fancy indexing functions when the dataset is a Zarr object then? |
|
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? |
| unique_sorted_order = slice(unique_sorted_order[0], unique_sorted_order[-1] + 1) | ||
| mini_data = time_series.data[t_ind_start:t_ind_stop, unique_sorted_order][:, inverse_sort] | ||
| else: | ||
| mini_data = np.array(time_series.data[t_ind_start:t_ind_stop])[:, unique_sorted_order][:, inverse_sort] |
There was a problem hiding this comment.
can we add a check here and have this work differently only for Zarr dataset objects? I'd prefer to use the simultaneous indexing approach for h5py datasets where we can so we don't load data into memory when we don't need to. I also think this could and probably should be refactored into a data utility function that can be used in other places
|
@alejoe91 can you also please add checks that this data indexing utility does indeed solve the problem for Zarr dataset objects? |
Turns out fancy indexing is not supported for
zarr.Datasetobjects and this was causing an issue in displaying timeseries correctly.This small PR fixes it by changing the behavior of the
timeseries.pyand:Not that for NWBwidgets in all cases we can slice directly, because of the channel slider.