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

[FEAT] use current stylesheet's color palette by default #92

Merged
merged 5 commits into from
Jul 8, 2024

Conversation

elephaint
Copy link
Contributor

  • Adds stylesheet argument to plot_series so that users can use matplotlibs pre-defined stylesheets.
  • Refactor of the plot and legend code a bit such that legend is always outside of the figure with sufficient space.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@jmoralez
Copy link
Member

I'm not sure about the argument, I don't see a lot of benefit from doing:

plot_series(..., stylesheet='some_stylesheet')

vs

plt.style.use('some_stylesheet')
plot_series(...)

or

with plt.style.context('some_stylesheet', after_reset=True):
    plot_series(...)

@elephaint
Copy link
Contributor Author

elephaint commented Jun 27, 2024

I'm not sure about the argument, I don't see a lot of benefit from doing:

plot_series(..., stylesheet='some_stylesheet')

vs

plt.style.use('some_stylesheet')
plot_series(...)

or

with plt.style.context('some_stylesheet', after_reset=True):
    plot_series(...)
  1. Convenience for users - the argument is easier to use; Anybody understands changing a function argument, nobody knows/understands setting stylesheets globally and/or with a wrapper.
  2. Your code isn't the same. The colour palette of the stylesheet is used differently in my code, as we might have much more plots than colors in the scheme of the stylesheet, so there's a linear segmentation.

@jmoralez
Copy link
Member

Convenience for users - the argument is easier to use

Maybe once but if they want the whole notebook to have the same style they have to pass the argument every time, as opposed to setting it once at the start. Also, people do a lot more complex things than setting the style, just look at what is done here

plt.style.use('grayscale') # fivethirtyeight  grayscale  classic
plt.rcParams['lines.linewidth'] = 1.5
dark_style = {
    'figure.facecolor': '#008080',  # #212946
    'axes.facecolor': '#008080',
    'savefig.facecolor': '#008080',
    'axes.grid': True,
    'axes.grid.which': 'both',
    'axes.spines.left': False,
    'axes.spines.right': False,
    'axes.spines.top': False,
    'axes.spines.bottom': False,
    'grid.color': '#000000',  #2A3459
    'grid.linewidth': '1',
    'text.color': '0.9',
    'axes.labelcolor': '0.9',
    'xtick.color': '0.9',
    'ytick.color': '0.9',
    'font.size': 12 }
plt.rcParams.update(dark_style)

I agree with the other changes, just not the stylesheet argument.

The colour palette of the stylesheet is used differently in my code

The palette change isn't related to the stylesheet, is it?

@elephaint
Copy link
Contributor Author

elephaint commented Jun 27, 2024

Convenience for users - the argument is easier to use

Maybe once but if they want the whole notebook to have the same style they have to pass the argument every time, as opposed to setting it once at the start. Also, people do a lot more complex things than setting the style, just look at what is done here

plt.style.use('grayscale') # fivethirtyeight  grayscale  classic
plt.rcParams['lines.linewidth'] = 1.5
dark_style = {
    'figure.facecolor': '#008080',  # #212946
    'axes.facecolor': '#008080',
    'savefig.facecolor': '#008080',
    'axes.grid': True,
    'axes.grid.which': 'both',
    'axes.spines.left': False,
    'axes.spines.right': False,
    'axes.spines.top': False,
    'axes.spines.bottom': False,
    'grid.color': '#000000',  #2A3459
    'grid.linewidth': '1',
    'text.color': '0.9',
    'axes.labelcolor': '0.9',
    'xtick.color': '0.9',
    'ytick.color': '0.9',
    'font.size': 12 }
plt.rcParams.update(dark_style)

I agree with the other changes, just not the stylesheet argument.

The colour palette of the stylesheet is used differently in my code

The palette change isn't related to the stylesheet, is it?

Ok, agree to remove the argument, you have me convinced.

Re. palette change: I was referring to the fact that we currently create a linear/gradient segmented color map out of the chosen palette in order to render the plot. If a user chooses a stylesheet globally, it (thus) does not use the colors of the stylesheet (but from the palette). So it's not precisely the same - the user chooses a stylesheet but the colors will be from the palette (which also has to be set every time), not the stylesheet. In my code I force the colors to be from the stylesheet too (by creating a linear segmented color map based on the colors from the stylesheet). Note that we can implement that too without the stylesheet argument - personally think that would be better (i.e. setting the colors based on the globally set stylesheet rather than the palette argument). But, minor detail. Let me know what you think, otherwise I'll just remove the stylesheet argument and update the PR as such.

@jmoralez
Copy link
Member

the user chooses a stylesheet but the colors will be from the palette (which also has to be set every time), not the stylesheet

In this PR if the palette is None then the colors are taken from the global style, right? So if a user sets the style to a different one it automatically adapts the colors instead of using the current default colormap (viridis). I agree with that (adding the None option and making it the default), it makes it easier to customize the full style of the plot (although not the dimensions which would be nice to have as well later on by accepting a matplotlib axes or similar).

@jmoralez jmoralez added the enhancement New feature or request label Jul 2, 2024
jmoralez
jmoralez previously approved these changes Jul 4, 2024
@elephaint elephaint merged commit cc06e63 into main Jul 8, 2024
18 checks passed
@elephaint elephaint deleted the feature/add_stylesheet branch July 8, 2024 17:51
@jmoralez jmoralez changed the title [FEAT] Add stylesheet argument [FEAT] use current stylesheet's color palette by default Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants