-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Update seaborn style #13680
Conversation
5d0428f
to
6e23079
Compare
|
||
@check_figures_equal(extensions=["png"]) | ||
def test_seaborn_style(fig_test, fig_ref): | ||
seaborn = pytest.importorskip('seaborn') |
There was a problem hiding this comment.
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.
Bringing this to 3.1 would be nice, but if there's no review, we can also postpone. |
|
6e23079
to
870ea97
Compare
|
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? |
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: |
There was a problem hiding this 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?
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 😃 . |
870ea97
to
973f7ac
Compare
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 |
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
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.... |
changed my mind based on discussion on this weeks call.
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. |
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. |
In principle, one could add any seaborn version's styles as 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, Those are just some thoughts from my side on the matter. Maybe they are helpful for further discussion. |
I don't think any of this needs a separate repository, it just needs some management. In this case, I'd be fine w/ |
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. |
FWIW my preference would be that if you're not going to update the styles, you should deprecate and remove them. |
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
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.