Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
Add easy style sheet selection#2236
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
dpsanders commentedJul 21, 2013
Fantastic news! - many thanks for this. 💯 I suggest |
dpsanders commentedJul 21, 2013
I merged your branch and installed |
tonysyu commentedJul 21, 2013
@dpsanders Most likely I should have added the BTW, it sounds like you might want to tweak your git workflow. You don't need to merge my branch to test it out (unless by merge, you meant pull). Just checkout the branch and test on there (never merge into master---only pull from the "official" master). This might be helpful: https://gist.github.com/piscisaureus/3342247 Also, if you run |
dpsanders commentedJul 21, 2013
On Sun, Jul 21, 2013 at 11:14 AM, Tony S Yunotifications@github.comwrote:
I'll give it a go
D.
Dr. David P. Sanders Profesor Titular "A" / Associate Professor dpsanders@gmail.com Cubículo / office:#414, 4o. piso del Depto. de Física Tel.: +52 55 5622 4965 |
ChrisBeaumont commentedJul 21, 2013
+1 on this A few outsider thoughts
|
ChrisBeaumont commentedJul 21, 2013
One other thought: if I'm parsing this correctly, it looks like styles update the current rcParams settings, but don't touch options that aren't specifically in the style sheet. I guess this enables the "chaining" behavior you talk about, but it also means that the state of rcParams after calling It might be nice to have an option to explicitly set unspecified style options to a default (the obvious choice being |
lib/matplotlib/style/core.py Outdated
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.
This would benefit from a more helpful error message ifs not in library
tonysyu commentedJul 21, 2013
@ChrisBeaumont Thanks for the feedback!
That's a great idea. Done!
I'm going to punt on this idea. It'd be simple to use the json file to update the settings, but ideally, it would be run through the same validation mechanism as normal rc files. Unfortunately, that validation mechanism is a bit tangled with the parser at the moment. I don't think it'd be hard to fix, but I'm not jumping at the opportunity to fix it myself ;)
Yeah, I would stick to the standard
True. You can just call |
ChrisBeaumont commentedJul 21, 2013
Good points. My main reason for the suggestion about an rcParams-specified default style is to pave the way to change the defaults in MPL -- if users could add a |
lib/matplotlib/style/core.py Outdated
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 hope you don't really need pyplot for this.
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.
Good point. Fixed.
efiring commentedJul 21, 2013
On 2013/07/21 10:15 AM, Tony S Yu wrote:
plt.rcdefaults() is part of the present confusing mess, with too many |
mdboom commentedJul 22, 2013
Thanks, Tony. This looks quite good. I'm going to second@ChrisBeaumont's suggestion that an rcParam to set the style would be good, to make the transition easier, as suggested. It's a tricky bootstrapping problem, however. If it's applied when it is read in, because then subsequent values in the matplotlibrc file would overwrite the style. Maybe that's correct, but it has potential for surprise and introduces some order-dependence on the rc files that we don't currently have. And then there's what to do with cyclical references to styles. All resolvable problems -- just tricky to get right I suspect. I don't like I agree that we want two ways of applying styles: one for chaining styles over top of the existing settings, and one that resets and then applies the style. Not sure on the best way to present that. @efiring: One of the complications with rcParams that has long irked me is that it combines style things with platform configuration. I've often felt that style belongs much closer to the plot -- not really in user-global configuration as we do now -- whereas the platform configuration stuff (which backend to use, and font formats to write out)do belong where they are. I'd love to see this separate out into two separate files (which would also solve the cyclical reference problem above, since the platform config would choose a style, but a style file could not choose another style (perhaps only inherit from another style). I don't think that would be hard to implement, but devising the right way to transition people is the hard part. |
ChrisBeaumont commentedJul 22, 2013
@mdboom agreed, the bootstrapping issue seems a little awkward, with three layers to apply (default values, matplotlibrc values, default style). For compatibility, I think this order is needed to start
For now, the default style is an empty called 'classic'. If the default style changes, people have the option of switching back to 'classic' in matplotlibrc. Long term, I agree it would be cleaner to pull the visual-level options out of defaultParams, and then load the default style in "explicit mode" (which applies the visual options currently in defaultParams). Likewise, matplotlibrc would not have visual-level options, so styles and matplotlibrc never conflict. The transition period would be awkward, however (you'd have to warn users that matplotlibrc values should not have visual level options, and then load them anyways after the default style is applied in explicit mode). |
lib/matplotlib/style/__init__.py Outdated
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.
is there a reason that this file doesn't just contain all the contents of core, instead of wildcard importing them?
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.
Just a matter of style, I guess. I prefer__init__.py modules to be fairly lightweight, but if that's frowned upon, I'm fine with changing it.
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.
also, aren't we doing the dot-style imports now? I also still don't like the import *.
ivanov commentedJul 22, 2013
I'm just jumping in here because it was mentioned on twitter, but will it be possible to either make a context manager for this, or re-use the |
mdboom commentedJul 22, 2013
@ivanov: Good idea on the context manager. Also, as a side note -- we should be able to wrap the |
tonysyu commentedJul 23, 2013
@ChrisBeaumont,@mdboom I'm convinced: It would be good to make style sheets an rcParam, but in the current state it'll be difficult to validate because of circular imports. I'm guessing that I need to add the stylelib directory to some sort of package data field in Updates:
|
WeatherGod commentedJul 23, 2013
I am not yet convinced that having a style rcParam makes any sense. Instead, why don't we jump on the other idea@mdboom had, which was the |
mdboom commentedJul 23, 2013
@WeatherGod: The tricky thing about my suggestion -- of separating style from other parameters -- is how best to transition to it, as@ChrisBeaumont pointed out:
But maybe we should bite the bullet and do it, despite its akwardness. |
tonysyu commentedJul 23, 2013
I agree that it would be good to separate style parameters from config parameters. Just to be clear though, this would be work for a separate PR, correct? (As part of that, removing the bulk of the rc logic from |
WeatherGod commentedJul 23, 2013
Understandable, but I would suggest holding off on a "style" parameter |
tonysyu commentedJul 23, 2013
I'd agree with holding off on a "style" parameter. In any case, a lot of rc-refactoring will need to be done before that. |
ChrisBeaumont commentedJul 23, 2013
I understand the concern, but it's too bad -- once a style with better defaults matures, it will be a shame to have to write 'style.use('better')' at the top of every script. I'd definitely be willing to help and/or spearhead the proposed rcParams refactoring, to keep momentum going on this -- though I may not be the most qualified person to tackle a big API change like this. |
tonysyu commentedJul 23, 2013
Sorry, I don't mean to suggest that we should punt on this indefinitely. I just think that smaller, more focused PRs are easier for everyone involved. (Plus, I don't think I want to lead the refactor :)
I think this could be split into two PRs: One just to refactor |
tacaswell commentedJul 24, 2013
This is far preferable than having to go back and touch every existing script that is broken by the 'better' defaults. I am in favor of treating anything more than near trivial tweaks to defaults as amajor api break (like bump to 2.0). If a change requires re-generating a test image, you are breaking the api, and I suspect that this will require a majority of the test images to be re-generated. That said, the ability easily save and set the styling is first order good, in part as protection against the defaults changing. [edit: gah I know grammer, I swear] |
dpsanders commentedJul 24, 2013
As far as I can see, the style module is still unusable from matplotlib master. |
ChrisBeaumont commentedJul 24, 2013
@dpsanders@tonysyu you probably want to add |
ChrisBeaumont commentedJul 24, 2013
@tacaswell I don't think that is the issue being discussed here. Instead, this is about enabling a kind of opt-in behavior where someone can specify a default style in their own |
dpsanders commentedJul 24, 2013
@pelson Great, thanks -- I tried that, but got the following error from error: package directory 'lib/matplotlib/stylematplotlib/testing' does not exist Any ideas? |
Conflicts:lib/matplotlib/tests/test_style.pyConflicts:lib/matplotlib/tests/test_style.py
removed some of the unneeded imports
End result of the experiment: The failure isn't related to the testsadded by this PR.
tonysyu commentedNov 17, 2013
Tests arealmost passing. Maybe Python 2.6 has an issue with a context-manager returning a generator? |
mdboom commentedNov 18, 2013
I've made a PR against this one with a Python 2.6 fix |
Fix for Python 2.6
tonysyu commentedNov 18, 2013
Woot! Tests passing! |
mdboom commentedNov 18, 2013
Merged. Thanks, Tony. This was a big one! |
ChrisBeaumont commentedNov 18, 2013
Phew! That was harder than I expected, but I'm glad this is now part of MPL |
tonysyu commentedNov 19, 2013
Thanks to everyone for all the improvements and fixes. It definitely took longer than expected, but it's great to have this in master. On to the rcparams refactor! (I'm not volunteering to lead that charge, but I'm happy to help where I can :). |
Add easy switching between rcParams based on the implementation inmpltools.style. Basically, just call
... to switch to a style sheet that sort of mimics ggplot's style.
Notes:
matplotlib/style/styleliband user files are stored in~/.matplotlib/stylelib.I choseStyle files in the style libraries have the extension*.mplrcas an extension for the style files, but I'm fine with changing that.*.style*.mplstyle.One thing I liked in the original implementation was the ability to chain style sheets, with each style sheet adding the parameters they set. The current implementation doesn't work like this becauserc_params_from_fileinitializes the defaultrcParamsand then updates the defaults from the file. Thus, updating using the result fromrc_params_from_filewill overwrite all values.