Closed ryanhammonds closed 3 years ago
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:
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:
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?
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.
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.
This updates
plot_time_series
to accept 2d arrays, in addition to a list of 1d arrays. Before this would not work:and it would require: