Skip to content

Update seaborn style #13680

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

Closed
wants to merge 1 commit into from
Closed

Conversation

timhoffm
Copy link
Member

@timhoffm timhoffm commented Mar 16, 2019

PR Summary

Following up on #13022 (comment)

seaborn has implemented some style changes in v0.9.0. This tries to get matplotlib styles back in sync with seaborn.

Current status: Done

  • update seaborn.mplstyle
  • update seaborn-[palette].mplstyle styles
  • update seaborn-[context].mplstyle styles
  • update other seaborn-[style].mplstyle styles
  • add a test for the styles
  • add a changelog entry

Review proposal

For review, I suggest to focus on the tests. They should make sure that the contents of the .mplstyle file matches the definitions in seaborn. I don't think it's feasible to cross-check every single parameter by hand.

@timhoffm timhoffm force-pushed the update-seaborn-style branch 4 times, most recently from 5d0428f to 6e23079 Compare March 17, 2019 10:56

@check_figures_equal(extensions=["png"])
def test_seaborn_style(fig_test, fig_ref):
seaborn = pytest.importorskip('seaborn')
Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't get check_figures_equal to work with the seaborn fixture, so I copied the checking code here explicitly.

@timhoffm timhoffm added this to the v3.1.0 milestone Mar 17, 2019
@timhoffm
Copy link
Member Author

Bringing this to 3.1 would be nice, but if there's no review, we can also postpone.

@ImportanceOfBeingErnest
Copy link
Member

  • Is it necessary to carry all those ...6.mplstyles around? One could argue that those seaborn styles are simply outdated and could be removed.
  • I'm not convinced that the missing rocket colormap should be replaced by Greys. In my eyes, either we think that this is a useful colormap and introduce it into matplotlib as well, or we should use the matplotlib default colormap instead.

@timhoffm timhoffm force-pushed the update-seaborn-style branch from 6e23079 to 870ea97 Compare March 17, 2019 12:51
@timhoffm
Copy link
Member Author

timhoffm commented Mar 17, 2019

  • ...6.mplstyle: Seaborn keeps their old 6-color palettes around using these names. I've just adopted their convention. I don't have a strong opinion on keeping or removing them.
  • rocket colormap: Using Greys in place of rocket is not new. I've taken that from the existing mplstyles. Here also, I don't have a strong opinion.

@jklymak
Copy link
Member

jklymak commented Mar 17, 2019

So won’t this unexpectedly change the style for anyone using these style sheets? How do they get the old styles back? I guess I wonder if these should not all have new names?

@timhoffm
Copy link
Member Author

Yes, this does change the styles a bit. But the same changes have occured in seaborn 0.9. I'd rather be consistent with the current seaborn naming than with the old one, even if that changes the plots.

The old styles (linewidth, textsize etc. have slightly changed) are not available anymore. The old palettes can be included using the ...6 styles: plt.style.use('seaborn-colorblind6'). Again, I'm just following what seaborn is doing here. I don't think it's worth adding different names or additional old styles on our side if seaborn doesn't. If users really want, they can copy the old mplstyle files.

tacaswell
tacaswell previously approved these changes Mar 17, 2019
Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

I am slightly concerned about the technical debt going forward that seaborn can make changes that fail our tests.

Can you add seaborn to the testing requirements files?

@timhoffm
Copy link
Member Author

timhoffm commented Mar 17, 2019

That's exactly the trade-off. We can strongly bind our tests to seaborn, so that they can make our tests fail, but we immediately recognize if we go out of sync. Or we do not run the tests by default (maybe even add a marker for that) and just check occasionally. I wasn't bold enough to require seaborn for the tests, but am happy to include it 😃 .

@timhoffm timhoffm force-pushed the update-seaborn-style branch from 870ea97 to 973f7ac Compare March 17, 2019 19:59
@jklymak
Copy link
Member

jklymak commented Mar 18, 2019

Yes, this does change the styles a bit. But the same changes have occured in seaborn 0.9. I'd rather be consistent with the current seaborn naming than with the old one, even if that changes the plots

I'm just not sure we should be mirroring another library's styles, or if we do, I'm not sure we should be worried about syncing them.

I use the ggplot.style sheet, and if it changed suddenly because ggplot changed, I'd be a bit miffed because I like it for what it is, not because its directly comparable to ggplot.

@timhoffm
Copy link
Member Author

I'm just not sure we should be mirroring another library's styles, or if we do, I'm not sure we should be worried about syncing them.

I use the ggplot.style sheet, and if it changed suddenly because ggplot changed, I'd be a bit miffed because I like it for what it is, not because its directly comparable to ggplot.

I think otherwise. If we take over the style with the same name, we should be in sync with the original. Our ggplot example states

"This example demonstrates the "ggplot" style, which adjusts the style to emulate ggplot".

In that sense, it would be misleading not to sync. If we don't want that, we should call it "hhplot" or "mpl-gg" and just state that it's inspired by ggplot.

@jklymak
Copy link
Member

jklymak commented Mar 18, 2019

In that sense, it would be misleading not to sync. If we don't want that, we should call it "hhplot" or "mpl-gg" and just state that it's inspired by ggplot.

I think that approach would be wiser rather than claiming that we are keeping in sync....

@tacaswell tacaswell modified the milestones: v3.1.0, v3.2.0 Mar 18, 2019
@tacaswell tacaswell dismissed their stale review March 18, 2019 19:28

changed my mind based on discussion on this weeks call.

@efiring
Copy link
Member

efiring commented Mar 18, 2019

On the March 18 phone call, there was general agreement that we don't want to change the existing style because that would disturb those relying on it. The dominant suggestion seems to be that people wanting to track current seaborn will need to simply install seaborn. An alternative, keeping versioned seaborn styles, did not get much support.

@timhoffm
Copy link
Member Author

timhoffm commented Mar 18, 2019

Too bad I couldn't make it to the call. 🙁

I don't think this is a wise decision. We are trying to protect users from seaborn style changes to keep backward-compatibility for existing figures. But these style changes are real. If people use seaborn, they also have to go along with the change.

Even more importantly, any future plot without a preexisting history will look different in seaborn and matplotlib-seaborn. I don't find that acceptable. If we don't want to adapt, let's please remove or rename the styles. Just keeping them will cause confusion in the future.

Btw. is there a way for packages to register styles? If so, it would be reasonable to move all seaborn styles to a separate package. Then, users have full control on which version they install and it's decoupled from matplotlib.

@ImportanceOfBeingErnest
Copy link
Member

In principle, one could add any seaborn version's styles as seaborn<version#>-<palette/context>.mplstyle.
On the other hand that would result in a lot of style sheet files.

In that sense, I see a bit of a parallel to the attempt to introduce the new tableau colors (#12009). That proposal (which did not even want to change any of the defaults) was declined with the argument that there are already too many colors in matplotlib. Introducing that many style sheets, while keeping the number of colors/colormaps low seems contradictory.

A solution may hence be the same as the one proposed in #12009, namely to externalize all the colors/colormaps/styles to some repository which people can import if they need it. Yet, such repo would still need to be created at some point, to deserve being used as argument not to improve current stylings.

That being said, of course people can import seaborn or import ggplot and set the styles with the help of those libraries. For seaborn, import seaborn; seaborn.set(...), for ggplot from ggplot.themes import theme_gray; plt.rcParams.update(theme_gray()._rcParams).
Also, one should be aware that it is impossible to replicate the look of those libraries in completeness. Seaborn changes the saturation of colors for certain plots independent of the cycler, and also manipulates the spines in certain cases. ggplot uses an internal version of a color cycler depending on the kind of plot and datatype to be shown.

Those are just some thoughts from my side on the matter. Maybe they are helpful for further discussion.

@jklymak
Copy link
Member

jklymak commented Mar 18, 2019

I don't think any of this needs a separate repository, it just needs some management.

In this case, I'd be fine w/ seaborn.2018.style being the old style, and the changes here. Just so long as someone who likes the current seaborn.style has a way to still be able to use it. In general I don't think we should take existing styles away or change them (unless there is a bug) without a way to get the old style back.

@dopplershift
Copy link
Contributor

I think the problem is that the semantics of our Seaborn styles, and thus the intention of our users in selecting them is undefined. Did they choose them so that the style will result in their code, from now until the end of time will match what Seaborn does? Or did they like the aesthetics of the seaborn style and matplotlib (unaware of the separate Seaborn package), and thus will be caught off-guard when matplotlib 3.x suddenly changes the results of their existing, unchanged code?

In the absence of any data on the matter, I'm going to assume the second interpretation, which matches how we've handled stability of matplotlib's styling in the past: matplotlib minor releases should not suddenly change their aesthetics.

@mwaskom
Copy link

mwaskom commented Apr 14, 2019

FWIW my preference would be that if you're not going to update the styles, you should deprecate and remove them.

@tacaswell tacaswell modified the milestones: v3.2.0, v3.3.0 Sep 5, 2019
@QuLogic QuLogic modified the milestones: v3.3.0, v3.4.0 May 2, 2020
@QuLogic QuLogic modified the milestones: v3.4.0, v3.5.0 Jan 22, 2021
@jklymak
Copy link
Member

jklymak commented Apr 15, 2021

This is being tracked by #14331, though I think we should respect @mwaskom 's wish and deprecate the styles if they are to not track seaborn.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants