[WIP] Trialized time series faceting #232

h-mayorquin · 2022-10-31T15:30:25Z

The idea is to add a widget for time series that can show trialized data both by filtering (selecting only data where a column -or more- are equal to one value) and faceting (showing the behavior across a grid when filtering for some values).

So far the widget works well as a stand alone:

But does not render properly when called from within:
nwb2widget(nwb)

Then there is no error but it never renders:

@bendichter

CodyCBakerPhD · 2022-10-31T16:12:15Z

Something I want to add for my data

faceting over data columns; it seems fast enough, would be nice way to visualize multiple ROI's at once

Something I need to add for my data

TimeInterval table selector; I have multiple tables, and may not always have a trials table

Something to discuss adding would be

observers for changes to data column selection; right now I have to push the render button each time, but if I want to rapidly scan through a bunch of ROI's this gets a bit annoying

A widget I'll make that harnesses parts of this (separate PR)

a 'heatmap' view of multiple ROI's plotted over the space of the image/pixel masks

CodyCBakerPhD · 2022-10-31T17:06:01Z

Also just noting that when you use the plotly interact with the faceting it adjusts each plot; is there maybe an option to control that? Or is it just because they all 'share' the x-axis? (basically I would want different x-axis scales each spanning the data range for each condition)

plotly_facet_all_axes_affected.mp4

nwbwidgets/timeseries.py

bendichter · 2022-10-31T17:24:19Z

@CodyCBakerPhD

Also just noting that when you use the plotly interact with the faceting it adjusts each plot; is there maybe an option to control that? Or is it just because they all 'share' the x-axis? (basically I would want different x-axis scales each spanning the data range for each condition)

I don't understand

CodyCBakerPhD · 2022-10-31T17:32:31Z

I don't understand

If you watch the video, as I attempt to zoom into one of those tiny data plotting regions for a single facet, it also zooms into that region for all the other facets. Likewise, from a zoomed out perspective, looking at all the data for one facet might just leave a tiny blip for the data on another facet. For those cases I'd prefer the plot to span the axis for each facet in a more reasonable way and then indicate the range on the x-ticks

I can also see why you'd want to be able to view everything aligned to the same x-axis for consistency, so maybe this could be a 'lock-axis' option or something

bendichter · 2022-10-31T17:37:01Z

@h-mayorquin it doesn't look like you have a way of controlling the time range. It looks like in @CodyCBakerPhD 's example you are plotting data for the next 100 seconds? That's way too long. There should be text boxes that allow you to control the time before and after the time of alignment, similar to the PSTH widget

h-mayorquin · 2022-10-31T19:50:38Z

@h-mayorquin it doesn't look like you have a way of controlling the time range. It looks like in @CodyCBakerPhD 's example you are plotting data for the next 100 seconds? That's way too long. There should be text boxes that allow you to control the time before and after the time of alignment, similar to the PSTH widget

Yes, in the Murthy conversion the trials are very short so is OK. I will see how to add this for the general case though.

Right now the trials are aligned to the beginning of the trial. What do you suggest doing when there is no data before. As in, here there is only data within the trials and they are non-contiguous? My default is returning none in this case.

h-mayorquin · 2022-10-31T19:56:16Z

I will take a look into the x-axis locking while zooming on faceting thing @CodyCBakerPhD

@bendichter any idea why would this not render from within the widget accordion but would render on its own?

CodyCBakerPhD · 2022-10-31T20:39:16Z

any idea why would this not render from within the widget accordion but would render on its own?

I've seen that sometimes if the figure isn't a go.FigureWidget or isn't properly otherwise embedded in a widgets.Output box, I/we can play around with it tomorrow

h-mayorquin · 2022-11-02T18:33:02Z

@h-mayorquin it doesn't look like you have a way of controlling the time range. It looks like in @CodyCBakerPhD 's example you are plotting data for the next 100 seconds? That's way too long. There should be text boxes that allow you to control the time before and after the time of alignment, similar to the PSTH widget

OK, I did this in the last commit.
@bendichter
Thinking more about this.
Before this widget ploted the traces within the trial, that is, the traces were plotted from (start_time, to stop_time) . I thought this made sense because start_time and stop_time are the meaningful interval for the trial (after all, that's the reason for them to be in the trials table).

I thought that the control that made sense was:

(start_time + "user_selected_time", stop_time + "another_user_selected_time)`:

Where both the user selected times (controlled by widgets) could be positive or negative.

But looking at the alignment control options in the PSTH:

It seems that the plotting interval is:
("time_chosen_for_alignment" + "user_selected_time", "time_chosen_for_alignment" + "another_user_selected_time)`:

Where the "time_chosen_for_alignment" is usually the start_time but it can be any other column that follows the convention of ending in _time.

Why does this make more sense than plotting over the whole trial? In what sense was the plotting interval in @CodyCBakerPhD example way too long you mentioned above?

h-mayorquin · 2022-11-02T18:37:42Z

@CodyCBakerPhD
I addressed your in person comment of disabling the filtering selection whe not usted in the last commit.
I think I can disable the x-locking behavior but I don't think it is a good idea to disable it in general. Plotly documentation advice against it and I guess they made it a default for a reason:
https://plotly.com/python/facet-plots/

We can have a checkbox that controls for whether the behavior is active or not if you and @bendichter think that is desirable.

CodyCBakerPhD · 2022-11-03T19:07:14Z

Plotly documentation advice against it and I guess they made it a default for a reason:
https://plotly.com/python/facet-plots/

We can have a checkbox that controls for whether the behavior is active or not if you and @bendichter think that is desirable.

Yeah an optional toggle is all I'm asking for there. Common axes are a good default, but in cases where some slices are very thin on certain facets it can make more sense to try to expand different axes to note the trends one might see on the zoomed-in scale

h-mayorquin · 2022-11-03T19:10:56Z

The widget so far:

showcase.mp4

CodyCBakerPhD · 2022-11-03T19:20:29Z

@h-mayorquin If you want more examples to test/develop with, here's a snippet for using my latest files (recommend using on DANDI Hub for optimal bandwidth speeds)

import h5py
import fsspec
from fsspec.implementations.cached import CachingFileSystem
from pynwb import NWBHDF5IO
from nwbwidgets.timeseries import TrializedTimeSeries

s3_url = "https://dandiarchive.s3.amazonaws.com/blobs/297/fd6/297fd6b9-4c28-48c4-8c86-e9ef2a0fb07d"
cfs = CachingFileSystem(
    fs=fsspec.filesystem("http"),
    cache_storage="/home/jovyan/fsspec_cache",  # Local folder for the cache
)
file_system = cfs.open(s3_url, "rb")
file = h5py.File(file_system)
io = NWBHDF5IO(file=file, load_namespaces=True)
nwbfile = io.read()

# Choose series
#roi_response_series = nwbfile.processing["ophys"]["DfOverF"].roi_response_series["NeuronDfOverF"]
roi_response_series = nwbfile.processing["ophys"]["Fluorescence"].roi_response_series["NeuronFluorescence"]

# Choose table
#target_table = nwbfile.processing["behavior"]["SwimIntervals"]
target_table = nwbfile.processing["behavior"]["ActivityStates"]

TrializedTimeSeries(time_series=roi_response_series, trials_table=target_table)

h-mayorquin · 2022-11-09T18:53:06Z

OK, I had to do some changes but now it renders with nwb2widget(nwb):

bendichter · 2022-11-09T18:57:42Z

@h-mayorquin great! What did you need to change?

h-mayorquin · 2022-11-09T19:14:17Z

@h-mayorquin great! What did you need to change?

There was a silent error because the nwb2widget function requires neurodata_vis_spec items to have a neurodata_vis_spec as an argument if they are callable. I added a router function (route_trialized_time_series) with this argument.

…atalystneuro/nwbwidgets into trialized_time_series_faceting

h-mayorquin · 2022-11-09T19:28:15Z

@CodyCBakerPhD I added a toggle to control the x-locking behavior in the last commit. Could you test it to confirm that is the behavior that you wanted?

h-mayorquin · 2022-11-09T19:35:16Z

nwbwidgets/ophys.py

-        )
-    else:
-        return neurodata_vis_spec[NWBDataInterface](df_over_f, neurodata_vis_spec)
+    return neurodata_vis_spec[NWBDataInterface](df_over_f, neurodata_vis_spec)


There is a problem here when we make the neurodata_vis_spec[RoiResponseSeries] an OrderedDict to include the TrializedTimeSeries as an option (see the diff in the view.py module).

Because show_df_over_f is a callable the function nwb2widget expects this output to be a visualization (not an ordered dict) and a naive extension of the visualization specification generates an error. The solution I implemented was to roll back the show_df_over_f to return the visualization for the whole Container/DataInterface (see the else in the diff representing the old code) and then the visualization can be properly display as a root of the lazy_tab for that module. That is how the widget is working right now.

The other option was to modify nwb2widget to handle the case when callables return OrderedDict but that seemed like a larger modification given that nwb2widget is such a central function and this might have non-tested side effects in other visualizations. I opted for the simple solution here as that only touches the visualizations concerning the RoiExtractors and DfOverF objects.

nwbwidgets/timeseries.py

bendichter · 2022-11-10T15:50:59Z

nwbwidgets/timeseries.py

+    return data.flatten()
+
+
+def trialize_time_series(


Could you use this function?

nwbwidgets/nwbwidgets/utils/timeseries.py

Line 266 in dba1400

def align_by_time_intervals(

Most of the function implemented there is to build the dataframe into the form where the faceting functionality of plotly can be used.

There is a place inside trialized_time_series where the data from trials is extracted and also the timestamps. For that part, after your last suggestion above, I took a look a the functions in that utils module for timeseries. I don't think they fit that well here without introducing more unnecessary computation.

Basically, right now I have a vectorized bisect function (as provided by numpy) which I use to extract the set of trialized indexes and then I use them on both the data and the timestamps. The functions in the utils module are non vectorized and they return the data in a list without the indexes which I need for the timestamps. Using align_by_time_intervals, for example, would reduce the performance here both by losing the vectorization and then by requiring some more computation to get the indexes of the timestamps.

I think the generalization would be to have a general vectorized function that return the trialized indexes. That hypothetical function then can be used both here and inside bisect_timeseries_by_times to avoid the per trial bisection there.

What do you think?

bendichter · 2022-11-10T15:51:37Z

nwbwidgets/timeseries.py

+    return empty_figure
+
+
+def calculate_moving_average_over_trials(df, moving_average_window):


This seems specific to your widget. Maybe it would be better as a static method.

I wanted to separate the logic of plot creation (here in the function build_faceting_figure) from the logic of the widget. In my view this separation of concerns (composition) makes it easier to modify and test the plotting function. Reason being:

It is easier for someone who does not know the function to get the plotting function and just use it by itself to understand its behavior. In opposition to this, in the AlignMultiTraceTimeSeriesByTrialsVariable you need to go very deep in the function to understand what is the plotting function.

The plotting function can be tested or modified with minimal modification to the widget class. I think keeping the points of contact between the plotting function and all the other widget class responsibilities small leads to better design.

Basically I view the widget class a place with the following responsabilities:

Data loading from the appropriate neuro data type.

Control definition (maybe can be done with composition using your concept of foreign controlers).

Handling the interaction between controls and the (external) plotting function.

What do you think? Are there downsides that I am not seeing for this separation?

That said, I can create a plotting class and make all the methods that are floating now by themselves methods of that class to give the code more coherence.

I think a better way to organize would be to separate calculation from plotting.

Thanks, this got me thinking:
#239

bendichter · 2022-11-10T15:51:41Z

nwbwidgets/timeseries.py

+    return df_sort
+
+
+def add_moving_average_traces(figure, df, facet_col, facet_row):


This seems specific to your widget. Maybe it would be better as a static method.

bendichter · 2022-11-10T15:51:50Z

nwbwidgets/timeseries.py

+    return figure
+
+
+def build_faceting_figure(df, facet_col, facet_row, data_label="data", trial_label="trial"):


This seems specific to your widget. Maybe it would be better as a static method.

bendichter · 2022-11-10T15:52:14Z

nwbwidgets/timeseries.py

+            self.figure_widget.update_xaxes(matches=matches)
+
+
+def route_trialized_time_series(time_series: TimeSeries, neurodata_vis_spec=None, **kwargs):


There is no routing going on here..

It would also be good if you subclassed TrializedTimeSeries to have proper labels for specific modalities

I was thinking that would be the role of the routing function. Initializing the visualization -for lack of a better term- to match the neurodata type that is specified for it in the visualization spec dictionary. Besides the column of the data that we are visualizing, what other labels do you think should be changed?

This is done in the last commit and in the routing function.

bendichter · 2022-11-29T14:02:34Z

@h-mayorquin the sorting looks right. Good thinking using natsort. Could you show a screenshot that displays this working correctly?

h-mayorquin · 2022-11-30T12:25:58Z

@h-mayorquin the sorting looks right. Good thinking using natsort. Could you show a screenshot that displays this working correctly?

Here is some recording of it working on the test data that I have been using:

sharing.mp4

h-mayorquin · 2022-12-06T12:30:29Z

@bendichter
I ordered the categories so they appear ordered when facetting.
I also added some more machinery to the routing function so when the time series is an RoiResponseSeries the choice over column says ROI and it says electrode when it is an ElectricalSeries in the commit just below.

h-mayorquin added 3 commits October 31, 2022 06:02

first draft

21bb54e

added first plot

97fecc5

add option when not available

89097cb

bendichter reviewed Oct 31, 2022

View reviewed changes

nwbwidgets/timeseries.py Outdated Show resolved Hide resolved

bendichter reviewed Oct 31, 2022

View reviewed changes

nwbwidgets/timeseries.py Outdated Show resolved Hide resolved

h-mayorquin added 2 commits November 2, 2022 09:11

added timeseries control

fd57c1c

hide filtering selection if not used

860624b

h-mayorquin added 4 commits November 2, 2022 12:54

format and keeping state with another variable

3493f2f

Merge branch 'master' into trialized_time_series_faceting

09a556a

added mechanism for not doubling timestamps

5269e42

added series scaling

eb1499a

h-mayorquin added 2 commits November 9, 2022 12:48

make it work with nwb2widget main function

68d6192

Merge branch 'master' into trialized_time_series_faceting

61db805

add toggle for x-locking behavior

761583b

Merge branch 'trialized_time_series_faceting' of https://github.com/c…

375c60b

…atalystneuro/nwbwidgets into trialized_time_series_faceting

h-mayorquin marked this pull request as ready for review November 9, 2022 19:28

h-mayorquin commented Nov 9, 2022

View reviewed changes

bendichter reviewed Nov 10, 2022

View reviewed changes

nwbwidgets/timeseries.py Outdated Show resolved Hide resolved

bendichter reviewed Nov 10, 2022

View reviewed changes

h-mayorquin added 2 commits November 11, 2022 15:19

use time series units

42ca8e7

displayed selection filter options filtered

4b26e16

separate conrtroller for visualization widget

0b4235c

h-mayorquin mentioned this pull request Nov 30, 2022

Proposal for VisualizationWidgets class organization #239

Open

ordered faceting columns and rows

d50e883

h-mayorquin added 2 commits December 6, 2022 13:54

added router for ROI and electrical series

4aed1b7

fix dropvalues

2bb7ba3

bendichter approved these changes Dec 6, 2022

View reviewed changes

bendichter merged commit 29b8d06 into NeurodataWithoutBorders:master Dec 6, 2022

CodyCBakerPhD deleted the trialized_time_series_faceting branch December 6, 2022 16:05

This was referenced Dec 14, 2022

Add pre commit configs #244

Merged

Fix CI not running on external forks #246

Merged

h-mayorquin mentioned this pull request Dec 16, 2022

Fix remaining tests #250

Merged

CodyCBakerPhD mentioned this pull request Feb 8, 2024

[Feature] Trial-aligned ROIResponseSeries flatironinstitute/neurosift#115

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Trialized time series faceting #232

[WIP] Trialized time series faceting #232

h-mayorquin commented Oct 31, 2022 •

edited

CodyCBakerPhD commented Oct 31, 2022 •

edited

CodyCBakerPhD commented Oct 31, 2022

bendichter commented Oct 31, 2022

CodyCBakerPhD commented Oct 31, 2022

bendichter commented Oct 31, 2022 •

edited

h-mayorquin commented Oct 31, 2022

h-mayorquin commented Oct 31, 2022

CodyCBakerPhD commented Oct 31, 2022

h-mayorquin commented Nov 2, 2022

h-mayorquin commented Nov 2, 2022

CodyCBakerPhD commented Nov 3, 2022 •

edited

h-mayorquin commented Nov 3, 2022

CodyCBakerPhD commented Nov 3, 2022 •

edited

h-mayorquin commented Nov 9, 2022 •

edited

bendichter commented Nov 9, 2022

h-mayorquin commented Nov 9, 2022

h-mayorquin commented Nov 9, 2022

h-mayorquin Nov 9, 2022 •

edited

bendichter Nov 10, 2022

h-mayorquin Nov 11, 2022

bendichter Nov 10, 2022

h-mayorquin Nov 11, 2022

h-mayorquin Nov 11, 2022

bendichter Nov 12, 2022

h-mayorquin Nov 30, 2022

bendichter Nov 10, 2022

bendichter Nov 10, 2022

bendichter Nov 10, 2022

bendichter Nov 10, 2022

h-mayorquin Nov 10, 2022 •

edited

h-mayorquin Dec 6, 2022

bendichter commented Nov 29, 2022

h-mayorquin commented Nov 30, 2022

h-mayorquin commented Dec 6, 2022 •

edited

		return empty_figure


		def calculate_moving_average_over_trials(df, moving_average_window):

		return df_sort


		def add_moving_average_traces(figure, df, facet_col, facet_row):

		return figure


		def build_faceting_figure(df, facet_col, facet_row, data_label="data", trial_label="trial"):

		self.figure_widget.update_xaxes(matches=matches)


		def route_trialized_time_series(time_series: TimeSeries, neurodata_vis_spec=None, **kwargs):

[WIP] Trialized time series faceting #232

[WIP] Trialized time series faceting #232

Conversation

h-mayorquin commented Oct 31, 2022 • edited

CodyCBakerPhD commented Oct 31, 2022 • edited

CodyCBakerPhD commented Oct 31, 2022

bendichter commented Oct 31, 2022

CodyCBakerPhD commented Oct 31, 2022

bendichter commented Oct 31, 2022 • edited

h-mayorquin commented Oct 31, 2022

h-mayorquin commented Oct 31, 2022

CodyCBakerPhD commented Oct 31, 2022

h-mayorquin commented Nov 2, 2022

h-mayorquin commented Nov 2, 2022

CodyCBakerPhD commented Nov 3, 2022 • edited

h-mayorquin commented Nov 3, 2022

CodyCBakerPhD commented Nov 3, 2022 • edited

h-mayorquin commented Nov 9, 2022 • edited

bendichter commented Nov 9, 2022

h-mayorquin commented Nov 9, 2022

h-mayorquin commented Nov 9, 2022

h-mayorquin Nov 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-mayorquin Nov 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendichter commented Nov 29, 2022

h-mayorquin commented Nov 30, 2022

h-mayorquin commented Dec 6, 2022 • edited

h-mayorquin commented Oct 31, 2022 •

edited

CodyCBakerPhD commented Oct 31, 2022 •

edited

bendichter commented Oct 31, 2022 •

edited

CodyCBakerPhD commented Nov 3, 2022 •

edited

CodyCBakerPhD commented Nov 3, 2022 •

edited

h-mayorquin commented Nov 9, 2022 •

edited

h-mayorquin Nov 9, 2022 •

edited

h-mayorquin Nov 10, 2022 •

edited

h-mayorquin commented Dec 6, 2022 •

edited