-
Notifications
You must be signed in to change notification settings - Fork 65
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] - Plotting updates #343
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
|
||
@savefig | ||
@style_plot | ||
def plot_autocorr(timepoints, autocorrs, labels=None, colors=None, ax=None, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only thought here is whether to include it in aperiodic.py or somewhere else, since acf isn't restricted to aperiodic signals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeh.... agreed. I wasn't sure where to put it, but since we already have neurodsp/aperiodic/autocorr
as the home of the function to compute autocorrelation, this seemed like the most consistent spot for the plot function. I don't know what a better name / place for these things is - so unless we want to re-org, move both I think this works best for now?
neurodsp/plts/spectral.py
Outdated
... 'sim_bursty_oscillation' : {'freq': 10}}) | ||
>>> freqs1, powers1 = compute_spectrum(sig1, fs=500) | ||
>>> freqs2, powers2 = compute_spectrum(sig2, fs=500) | ||
>>> plot_spectra_3D([freqs1, freqs2], [powers1, powers2]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed the z-axis label is cutoff when I copy/paste the example into a notebook. I tried plt.tight_layout() but it gave an error about the size of the fig not having large enough margins. Maybe a solution would be to add an ax
or fig
kwarg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm, this seems to be an issue with how jupyter notebooks do inline plotting:
matplotlib/matplotlib#28117
When saving out, the figure looks fine, so I don't think we are messing up the plot per se. I'm not sure if there's an action item here - I don't think we want to overtune too much to address a quirk in notebooks.
More broadly, I do think it makes sense to add more access to ax / fig - so I've added a check_ax_3D
that will allow for managing / specifying 3D axes.
Also, it seems with the current notebook quirk, changing the zoom can fix the render (https://stackoverflow.com/questions/77577613/matplotlib-3d-plot-z-label-cut-off), so, for example the following make the whole plot visible: plt.gca().set_box_aspect(None, zoom=0.80)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added an argument to plot_spectra_3d
to be able to pass in a zoom argument, so now in the notebook case one can set the scaling directly as wanted when defining the plot
This PR updates and extends some plotting code. It adds to neurodsp some code I developed in the AperiodicHistory project, which I think are general enough to be added her.
Main updates:
prepare_multi_plot
which consolidates the same process we had in multiple places for supporting plots that may (or may not) have multiple inputs (e.g. plotting multiple spectra on the same plot)plot_autocorr
plotplot_spectra_3d
plot