Nixtla / utilsforecast

https://nixtlaverse.nixtla.io/utilsforecast
Apache License 2.0
42 stars 6 forks source link

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

Closed elephaint closed 2 months ago

elephaint commented 3 months ago
review-notebook-app[bot] commented 3 months ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

jmoralez commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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).