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 style managment #176

Merged
merged 9 commits into from
Apr 12, 2021
Merged

[ENH] Plot style managment #176

merged 9 commits into from
Apr 12, 2021

Conversation

ryanhammonds
Copy link
Contributor

This PR addresses #174 and expands plot styling similar to neurodsp. Here is a summary of the changes:

  • plot_style and savefig added, based on neurodsp's implmentation.

  • The plot method of FOOOF and FOOOFGroup, accepts kwargs for these new decorators.

  • plts.aperiodic, plts.periodic, plts.error, plts.spectra, are decorated with plot_style and save_fig.

  • I didn't decorate the functions in plts.annotate. Only plots with one axes were decorated, with the exception of FOOOFGroup.plot. This was because > 1 axes with varying number of lines per axis became increasing difficult to decorate properly.

  • Related tests have been updated.

@TomDonoghue
Copy link
Member

So, functionally, something in here is not playing well plot functions attached to the FOOOF object.

Example code to show the issue:

from fooof import FOOOF
from fooof.sim import gen_power_spectrum
from fooof.plts.utils import check_ax

freqs, powers = gen_power_spectrum([3, 50], [1, 1], [10, 0.2, 2])

fm = FOOOF(verbose=False)
fm.fit(freqs, powers)

ax = check_ax(None, figsize=(6, 5))
fm.plot(save_fig=True, file_name='test.pdf', ax=ax)

Output:
Screen Shot 2020-07-18 at 12 38 41 AM

Issues:

  • color scheme has changed (and we don't want that)
  • this call should save out, but it does not

Note that if you call the plot function directly, the color scheming still goes wrong, but the saving works

from fooof.plts.fm import plot_fm
plot_fm(fm, save_fig=True, file_name='test.pdf')

I think this is an interaction between all the different style functions. There's a lot of decorating going on (my fault). I don't understand the save thing. For the color, I think there's an interaction between all the styling that isn't going well. I think a general ToDo here might be to merge & consolidate a bit between the current FOOOF styling approach, and the we aspects e are trying to add, because they are not totally playing nice together, and I feel like we're getting a lot of complexity here.

@TomDonoghue
Copy link
Member

I think an issue here is that for some of the plots we end up with two styling approaches - using both style_plot the new decorator, and the old plot_style & check_n_style. I think we really need to consolidate down to one approach.

So, here's something relevant. The current / old FOOOF styling does check and style because it takes arguments. This is because the style func needs to know if axes are in log space. At the time I couldn't figure out how to do so without arguments passed in from inside the plot function, which doesn't play nice with the decorators.

But - brainwave. We can avoid arguments to the spectrum style function by annotating the plot axis with custom attributes!

Here's a simple code example to demonstrate the point:

import matplotlib.pyplot as plt

def custom_style(ax):
    
    if getattr(ax, 'my_property', False):
        ax.set_title('Custom Style!')

def my_plot(x, y):
    _, ax = plt.subplots()
    ax.plot(x, y)
    ax.my_property = True
    custom_style(ax)
    
my_plot([1, 2], [3, 4])

I think this is quite relevant here, because we can try and do away with the whole check_n_style thing. plot_spectrum can be applied through the decorator process, hopefully, and we can potentially do something with setting custom attributes to propagate needed information to the style funcs. (This is also possibly a bad idea).

Anyways - just thoughts so far - I think we do need to simplify plot style management overall here, which I'm hoping will address the current bugs along the way.

@ryanhammonds
Copy link
Contributor Author

ryanhammonds commented Jul 27, 2020

I've spent a few days trying to figure out how to get everything to play nicely together with your suggestion and it's been difficult. There is a lot going on in the ways the two packages creates plots. I agree that we need to consolidate/simplify to one approach.

@ryanhammonds
Copy link
Contributor Author

ryanhammonds commented Aug 6, 2020

Hi @TomDonoghue, I finally figured this one out. I took your advice and removed plot_style & check_n_style in favor of decorators.

The original color issue you noted should be resolved. I checked other functionality by rebuilding the site and ensuring all graphs were the same. Everything should be behave like ndsp now.

I also updated tests to save plots (#180).

@TomDonoghue TomDonoghue mentioned this pull request Apr 12, 2021
@fooof-tools fooof-tools deleted a comment from codecov-io Apr 12, 2021
@fooof-tools fooof-tools deleted a comment from codecov-io Apr 12, 2021
@TomDonoghue
Copy link
Member

Okay, @ryanhammonds - sorry for the ludicrous amount of delay on me getting back to this one. It just seemed like a slightly intimidating PR to get into, since it had a lot of changes and seemed like it would take a lot of time, so I never quite got around to it.

Luckily, I was generally wrong that it was intimidating - overall, this update is really nice, and really clean. Thanks a ton for digging through all this and doing the work on this one - I'm super happy with the result. I did a couple small lints, some small tweaks, and slightly extended the styling to collection plot elements. I think this is all good now - to get things moving, I'm going to merge this in now!

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