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] - Plot updates & extensions #319

Merged
merged 23 commits into from
Feb 20, 2024
Merged

[ENH] - Plot updates & extensions #319

merged 23 commits into from
Feb 20, 2024

Conversation

TomDonoghue
Copy link
Member

@TomDonoghue TomDonoghue commented Aug 29, 2023

Some additions & tweaks to plotting including:

  • add a plot for multiple time series (plotted with a vertical offset)
  • add a combined 'timeseries & psd' plot
  • style tweaks that allow for settings ticks and ticklabels through plot_style
  • update styling descriptions and implementation, following fooof updates ([MNT] - Make grid an updateable argument in PSD plots fooof-tools/fooof#300)
  • add tight_layout as a custom plot style, meaning it can be passed in as an argument to turn on/off, and also apply with warning suppression, to stop annoying warnings that the plot layout has changed

The upshot of the style update is that something like this now works:

# Plot with a neurodsp plot function with `plot_style`, turning off xticks
plot_function(data, xticks=[])

Note: the tests on newer versions of python were failing on a doctest, where an answer that used to come out at 2.0 is now 2.000000000000026 (or something) - so I added rounding to this check.

@TomDonoghue TomDonoghue added the 2.3 Updates to go into a 2.3.0. label Aug 31, 2023
@TomDonoghue TomDonoghue changed the title [WIP] - Plot updates & extensions [ENH] - Plot updates & extensions Sep 7, 2023
@TomDonoghue TomDonoghue requested review from ryanhammonds and removed request for ryanhammonds February 14, 2024 02:12
Copy link
Member

@ryanhammonds ryanhammonds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I added a few optional comments. Something to think about for the future though is a separate plotting package the signals/psds, as the plotting functionality here grows.

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

Thanks for checking @ryanhammonds! I made some updates based on your comments, if you want to check the update for the multi plot in particular (since what the API for that should be is not so obvious).

And yeh, I agree - as things grow, we should definitely consider "what belongs where" and if/when we might consider splitting things out from ndsp. I've been meaning to do a proper sweep of code for a bit, so maybe let's check in on this sometime soon, as we check in on what goes into 2.3, what goes after, and any things we need to figure out on the roadmap otherwise.

@TomDonoghue TomDonoghue merged commit d33fff2 into main Feb 20, 2024
7 checks passed
@TomDonoghue TomDonoghue deleted the nplts branch February 20, 2024 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.3 Updates to go into a 2.3.0.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants