Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed

Conversation

timhoffm
Copy link
Member

@timhoffmtimhoffm commentedMar 16, 2019
edited
Loading

PR Summary

Following up on#13022 (comment)

seaborn has implemented somestyle 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.

@timhoffmtimhoffmforce-pushed theupdate-seaborn-style branch 4 times, most recently from5d0428f to6e23079CompareMarch 17, 2019 10:56

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

Choose a reason for hiding this comment

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

I didn't getcheck_figures_equal to work with theseaborn fixture, so I copied the checking code here explicitly.

@timhoffm
Copy link
MemberAuthor

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 missingrocket colormap should be replaced byGreys. 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
Copy link
MemberAuthor

timhoffm commentedMar 17, 2019
edited
Loading

  • ...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: UsingGreys in place ofrocket is not new. I've taken that from the existing mplstyles. Here also, I don't have a strong opinion.

@jklymak
Copy link
Member

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
MemberAuthor

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 changesMar 17, 2019
Copy link
Member

@tacaswelltacaswell 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
MemberAuthor

timhoffm commentedMar 17, 2019
edited
Loading

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

@jklymak
Copy link
Member

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 theggplot.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
MemberAuthor

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

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

@tacaswelltacaswell modified the milestones:v3.1.0,v3.2.0Mar 18, 2019
@tacaswelltacaswell dismissed theirstale reviewMarch 18, 2019 19:28

changed my mind based on discussion on this weeks call.

@efiring
Copy link
Member

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
MemberAuthor

timhoffm commentedMar 18, 2019
edited
Loading

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 asseaborn<version#>-<palette/context>.mplstyle.
On the other hand that would result ina 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 ggplotfrom 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

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

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

@timhoffmtimhoffm mentioned this pull requestMay 26, 2019
@tacaswelltacaswell modified the milestones:v3.2.0,v3.3.0Sep 5, 2019
@QuLogicQuLogic modified the milestones:v3.3.0,v3.4.0May 2, 2020
@QuLogicQuLogic modified the milestones:v3.4.0,v3.5.0Jan 22, 2021
@jklymak
Copy link
Member

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 freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@ImportanceOfBeingErnestImportanceOfBeingErnestImportanceOfBeingErnest left review comments

@tacaswelltacaswelltacaswell left review comments

Assignees
No one assigned
Projects
None yet
Milestone
v3.5.0
Development

Successfully merging this pull request may close these issues.

8 participants
@timhoffm@ImportanceOfBeingErnest@jklymak@efiring@dopplershift@mwaskom@tacaswell@QuLogic

[8]ページ先頭

©2009-2025 Movatter.jp