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

[ENH] 2d Array Plotting #246

Merged
merged 3 commits into from
Feb 10, 2021
Merged

[ENH] 2d Array Plotting #246

merged 3 commits into from
Feb 10, 2021

Conversation

ryanhammonds
Copy link
Member

This updates plot_time_series to accept 2d arrays, in addition to a list of 1d arrays. Before this would not work:

import numpy as np
from neurodsp.plts import plot_time_series

n_sigs = 10
fs = 1000
n_seconds = 10

sigs = np.repeat(np.arange(n_sigs), int(n_seconds * fs))
sigs = sigs.reshape((n_sigs, int(n_seconds * fs)))

times = np.arange(0, len(sigs[0])/fs, 1/fs)

plot_time_series(times, sigs)

and it would require:

plot_time_series(times, list(sigs))

neurodsp/plts/time_series.py Outdated Show resolved Hide resolved
neurodsp/plts/time_series.py Show resolved Hide resolved
neurodsp/plts/time_series.py Show resolved Hide resolved
neurodsp/plts/time_series.py Outdated Show resolved Hide resolved
@TomDonoghue
Copy link
Member

TomDonoghue commented Feb 5, 2021

Yeh, this is a totally sensible update. I've made some review comments for condensing etc. Also: maybe extend the test to test the case of having a 2d array input? Also, there are some 'secondary' functions which use plot_time_series (such as plot_bursts and plot_instantaneous_measure - can you have a quick check that this update doesn't induce anything weird with their outputs?

Here is a slightly different example in which this update does help to plot things cleanly:

from neurodsp.sim import sim_oscillation
from neurodsp.plts import plot_time_series
from neurodsp.utils import create_times

n_seconds, fs = 2, 1000

powers = [0, 0.25, 0.5, 0.75, 1]

times = create_times(n_seconds, fs)
sigs = np.array([sim_oscillation(n_seconds, fs, 10, variance=power) for power in powers])

plot_time_series(times, sigs)

Output:
Screen Shot 2021-02-05 at 12 50 25 AM

This is a slightly different from the notion of plotting something like 'multiple channels':

from neurodsp.sim import sim_oscillation
from neurodsp.plts import plot_time_series
from neurodsp.utils import create_times

n_seconds, fs = 2, 1000
powers = [0, 0.25, 0.5, 0.75, 1]

times = create_times(n_seconds, fs)
sigs = np.array([sim_oscillation(n_seconds, fs, 10, variance=power) + 2*ind for ind, power in enumerate(powers)])

plot_time_series(times, sigs)

output:
Screen Shot 2021-02-05 at 12 54 44 AM

Note, for example, that the y-axis labels and ticks don't really make sense in this case.

The point being: I think we should merge this update in for the current function, but this also perhaps highlights that we should add a different / new function for doing 'multi-channel'? This feels like it could be useful, though perhaps somewhat out of scope?

@ryanhammonds
Copy link
Member Author

ryanhammonds commented Feb 6, 2021

Thanks for the review! I went a little wild with trying to avoid to use repeat. I've incorporated your feedback.

This plays nicely with plot_instantaneous_measure. And doesn't change anything with plot_bursts - it only took a 1d array and that hasn't changed. A list or 2d array will break plot_bursts. I could change that if you thinks it's useful. I'll take some playing around with since vstacking sig and bursts doesn't work.

I think a multi-channel plotting function would be useful and wouldn't be too much work to write in but might overlap with MNE. I took a stab at this in ndspflow with plotly - it ended up not working super well for large arrays there since there was too much interactivity and something deep in plotly's codebase/dependencies caused slow loading and poor performance for what I was trying to do.

@TomDonoghue
Copy link
Member

If this doesn't change anything with instaneous, and bursts, I think we're good. I don't think it's an issue that bursts doesn't accept 2d - it's not even clear what it should do in that case, and it's documented to only take in a 1d array.

With that, the code & update in this PR all looks good to me, you can merge it in @ryanhammonds!

In terms of the multi-channel plotting (as a potential separate thing, not for this PR) - I agree, it could be useful, and try it out if you want - but it's not a strong priority necessarily.

@ryanhammonds ryanhammonds merged commit 88f9694 into main Feb 10, 2021
@TomDonoghue TomDonoghue deleted the plots branch February 11, 2021 00:45
@ryanhammonds ryanhammonds mentioned this pull request May 3, 2021
26 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants