Add easy style sheet selection #2236

Merged
merged 36 commits into from Nov 18, 2013

Projects

None yet
@tonysyu
Contributor
tonysyu commented Jul 21, 2013

Add easy switching between rcParams based on the implementation in mpltools.style. Basically, just call

from matplotlib import style
style.use('ggplot')

... to switch to a style sheet that sort of mimics ggplot's style.

Notes:

  • In the current implementation, shipped style files are stored in matplotlib/style/stylelib and user files are stored in ~/.matplotlib/stylelib.
  • I chose *.mplrc as an extension for the style files, but I'm fine with changing that. Style files in the style libraries have the extension *.style *.mplstyle.
  • Ideally there would an rc parameter (or some other mechanism) to easily add search paths for style files
  • 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 because rc_params_from_file initializes the default rcParams and then updates the defaults from the file. Thus, updating using the result from rc_params_from_file will overwrite all values.
@dpsanders

Fantastic news! - many thanks for this. 💯
Apologies for not pulling this in myself -- I am still getting to grips with the code.
(But glad that my "gentle" prodding worked ;) )

I suggest .style as the extension for the style files.

@dpsanders

I merged your branch and installed matplotlib, but there is no style submodule available to import from matplotlib.

@tonysyu
Contributor
tonysyu commented Jul 21, 2013

@dpsanders Most likely I should have added the style package to some list for installation. One of the matplotlib devs will have to enlighten me on where this would be.

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 python setup.py develop, then you can avoid re-installing (unless there are compiled extensions) when testing out different branches.

@dpsanders

On Sun, Jul 21, 2013 at 11:14 AM, Tony S Yu notifications@github.comwrote:

@dpsanders https://github.com/dpsanders Most likely I should have added
the style package to some list for installation. One of the matplotlib
devs will have to enlighten me on where this would be.

I am a novice at this stuff, but a quick Google threw up this, which seems
to be the answer:

http://stackoverflow.com/questions/16020725/python-setup-install-one-module-as-a-sub-module-of-another-module

I'll give it a go

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 python setup.py develop, then you can avoid
re-installing (unless there are compiled extensions) when testing out
different branches.

Ah, many thanks -- that will make my life much easier :)

D.


Reply to this email directly or view it on GitHubhttps://github.com/matplotlib/matplotlib/pull/2236#issuecomment-21312409
.

Dr. David P. Sanders

Profesor Titular "A" / Associate Professor
Departamento de Física, Facultad de Ciencias
Universidad Nacional Autónoma de México (UNAM)

dpsanders@gmail.com
http://sistemas.fciencias.unam.mx/~dsanders

Cubículo / office: #414, 4o. piso del Depto. de Física

Tel.: +52 55 5622 4965

@ChrisBeaumont
Contributor

+1 on this

A few outsider thoughts

  1. It would be easy to extend style.use to accept paths or urls, in addition to style names. Urls would be especially nice, since people could peruse nbviewer/github for good styles, and try them out without remembering the search path for style.use.
  2. Since rcParams is essentially a dict, It would be easy to accept json style sheets (e.g. https://raw.github.com/CamDavidsonPilon/Probabilistic-Programming-and-Bayesian-Methods-for-Hackers/master/styles/bmh_matplotlibrc.json)
  3. Is it already possible to configure MPL to use a style by default? If not, perhaps there should be an rcParams option like 'style.default', to specify this?
@ChrisBeaumont
Contributor

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 style.use is not always obvious. For example:

use('ggplot')  # image.cmap not modified, = 'jet'
use('greyscale') # image.cmap = 'gray'
use('ggplot') # image.cmap='gray'

It might be nice to have an option to explicitly set unspecified style options to a default (the obvious choice being rcParamsDefault or rcParamsOrig).

@ChrisBeaumont ChrisBeaumont commented on an outdated diff Jul 21, 2013
lib/matplotlib/style/core.py
+STYLE_FILE_PATTERN = re.compile('([A-Za-z._-]+).style$')
+
+
+def use(name):
+ """Use matplotlib rc parameters from a pre-defined name or from a file.
+
+ Parameters
+ ----------
+ name : str or list of str
+ Name of style. For list of available styles see `style.available`.
+ If given a list, each style is applied from first to last in the list.
+ """
+ if np.isscalar(name):
+ name = [name]
+ for s in name:
+ plt.rcParams.update(library[s])
@ChrisBeaumont
ChrisBeaumont Jul 21, 2013 Contributor

This would benefit from a more helpful error message if s not in library

@tonysyu
Contributor
tonysyu commented Jul 21, 2013

@ChrisBeaumont Thanks for the feedback!

It would be easy to extend style.use to accept paths or urls, in addition to style names.

That's a great idea. Done!

Since rcParams is essentially a dict, It would be easy to accept json style sheets

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

Is it already possible to configure MPL to use a style by default?

Yeah, I would stick to the standard ~/.matplotlib/matplotlibrc file for this type of thing.

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 style.use is not always obvious.

True. You can just call plt.rcdefaults() to do what you're suggesting. I'm thinking about adding style.reset() as an alias, but matplotlib already has too many names for the same thing so I'm hesitant.

@ChrisBeaumont
Contributor

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 style.default : classic line to their rcParams, then they could maintain a stable set of rcParams even if the defaults change. From discussions on the mailing list, such functionality sounds like a prerequisite for any proposed change to the defaults.

@efiring efiring and 1 other commented on an outdated diff Jul 21, 2013
lib/matplotlib/style/core.py
@@ -0,0 +1,119 @@
+"""
+Core functions and attributes for the matplotlib style library:
+
+``use``
+ Select style sheet to override the current matplotlib settings.
+``available``
+ List available style sheets.
+``library``
+ A dictionary of style names and matplotlib settings.
+"""
+import os
+import re
+
+import numpy as np
+import matplotlib.pyplot as plt
@efiring
efiring Jul 21, 2013 Member

I hope you don't really need pyplot for this.

@tonysyu
tonysyu Jul 21, 2013 Contributor

Good point. Fixed.

@efiring
Member
efiring commented Jul 21, 2013

On 2013/07/21 10:15 AM, Tony S Yu wrote:

You can just call |plt.rcdefaults()| to do what you're suggesting. I'm
thinking about adding |style.reset()| as an alias, but matplotlib
already has too many names for the same thing so I'm hesitant.

plt.rcdefaults() is part of the present confusing mess, with too many
rc* names that are too hard to remember and that don't clearly indicate
what they are or do. style.reset() is clearer, but I think we need to
think longer term, about how to improve the pyplot layer itself.

@mdboom
Member
mdboom commented Jul 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 style as an extension. It's too generic, and these files are very matplotlib-specific. I'd prefer mplstyle (or something better than that).

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
Contributor

@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

  1. rcParams populated with rcsetup.defualtParams
  2. matplotlibrc values applied
  3. default style applied in "implicit mode" (i.e. values not explicitly named are not overridden)

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

@ivanov ivanov and 2 others commented on an outdated diff Jul 22, 2013
lib/matplotlib/style/__init__.py
@@ -0,0 +1,2 @@
+from core import *
@ivanov
ivanov Jul 22, 2013 Member

is there a reason that this file doesn't just contain all the contents of core, instead of wildcard importing them?

@tonysyu
tonysyu Jul 23, 2013 Contributor

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.

@WeatherGod
WeatherGod Sep 18, 2013 Member

also, aren't we doing the dot-style imports now? I also still don't like the import *.

@ivanov
Member
ivanov commented Jul 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 with mpl.rc_context(...) manager for this?

@mdboom
Member
mdboom commented Jul 22, 2013

@ivanov: Good idea on the context manager.

Also, as a side note -- we should be able to wrap the xkcd command in this, and it wouldn't be a special one-off function any more.

@tonysyu
Contributor
tonysyu commented Jul 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 setup.py, but it's not clear to me where.

Updates:

  • I added a context manager as suggested by @ivanov
  • I switched the extension to *.mplstyle.
  • Tests!
@WeatherGod
Member

I am not yet convinced that having a style rcParam makes any sense.
Conceptually speaking, the matplotlibrc file is more like the style, and
having multiple versions of these files is like having multiple styles to
choose from. The circular logic involved in implementing a style rcParam
should be a huge red warning sign saying "you are thinking this wrong!".

Instead, why don't we jump on the other idea @mdboom had, which was the
separation of style-type parameters from the non-style type parameters
(like the default backend and such), and use the "style" parameter in that
configuration file to point to the desired configuration file that contains
the style parameters?

@mdboom
Member
mdboom commented Jul 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:

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

But maybe we should bite the bullet and do it, despite its akwardness.

@tonysyu
Contributor
tonysyu commented Jul 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 __init__.py and the mixing of data and logic in rcsetup.py would be very desirable---but maybe quite a bit of work.)

@WeatherGod
Member

Understandable, but I would suggest holding off on a "style" parameter
until that split happens. It just makes zero sense to me until the split is
made.

@tonysyu
Contributor
tonysyu commented Jul 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
Contributor

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
Contributor
tonysyu commented Jul 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

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

I think this could be split into two PRs: One just to refactor rcsetup.py and the rc parts of __init__.py into something a bit cleaner but with no API changes, and then the second part would be separating styles from config params. That's how I would do it at least.

@tacaswell
Member

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.

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 a major 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

As far as I can see, the style module is still unusable from matplotlib master.
Does anybody have a fix to register it -- @mdboom ?

@ChrisBeaumont
Contributor

@dpsanders @tonysyu you probably want to add matplotlib.style to the list in setupext.Matplotlib.get_packages, and style/stylelib/* to get_package_data in the same class (though I don't know if styles belong in mpl-data instead)

@ChrisBeaumont
Contributor

@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 matplotlibrc. That would make it easier for MPL to develop alternative sets of rcParams that people could use by default if they want, without breaking test images or legacy user code.

@dpsanders

@pelson Great, thanks -- I tried that, but got the following error from python setup.py build:

error: package directory 'lib/matplotlib/stylematplotlib/testing' does not exist

Any ideas?

@ChrisBeaumont
Contributor

add a comma :)

@dpsanders

Ouch, my bad :S
Still no stye submodule available, though...

On Wed, Jul 24, 2013 at 8:46 AM, Chris Beaumont notifications@github.comwrote:

add a comma :)


Reply to this email directly or view it on GitHubhttps://github.com/matplotlib/matplotlib/pull/2236#issuecomment-21485475
.

Dr. David P. Sanders

Profesor Titular "A" / Associate Professor
Departamento de Física, Facultad de Ciencias
Universidad Nacional Autónoma de México (UNAM)

dpsanders@gmail.com
http://sistemas.fciencias.unam.mx/~dsanders

Cubículo / office: #414, 4o. piso del Depto. de Física

Tel.: +52 55 5622 4965

@tonysyu
Contributor
tonysyu commented Jul 25, 2013

@ChrisBeaumont Thanks for the tip on configuring the setup. This seems to install properly on my system.

@dpsanders

I checked out @tonysyu 's branch with @ChrisBeaumont 's modifications, but still no matplotlib.style submodule available. Not sure what I'm doing wrong. (Well, I am sure: nothing... :P) Is there an installation log file or something I can look at?

@ChrisBeaumont
Contributor

@tonysu It doesn't look like your tests are being run by Travis -- I think you want to move them to lib/matplotlib/tests, as that's where everything else seems to be. It looks like you should also update the default_test_modules list in __init__.py.

@ChrisBeaumont
Contributor

@dpsanders let's talk about this off-line, until we diagnose if this is a problem with the PR or some one-off thing

@dpsanders

Apologies, I was being caught out by the fact that style does not appear in dir(matplotlib) nor in autocompletion.
It is indeed there and works perfectly -- style.use( "ggplot") looks stunning!

How can this dir / autocomplete bug be fixed?

Apart from that, ready to merge!

@tonysyu tonysyu referenced this pull request in tonysyu/mpltools Jul 29, 2013
Closed

support for ipython notebook styles #12

@Tillsten
Contributor

any reason this is not merged?

@mdboom
Member
mdboom commented Sep 10, 2013

Well, re-reading this, I think we got bogged down by the fact that rcParams aren't all really relevant to style... Maybe we should get this merged and play with it and see how things really play out in practice.

@ChrisBeaumont, @tonysyu: Any thoughts?

@tonysyu: Would you mind rebasing?

@ChrisBeaumont
Contributor

+1 on merging this, and tackling the questions about refactoring rcParams in a separate issue

@ChrisBeaumont
Contributor

Also see my previous comment about making sure the tests are discoverable

@tonysyu
Contributor
tonysyu commented Sep 11, 2013

Argh! Sorry, I recently reformatted my computer, and I haven't been able to get matplotlib building correctly on my system. (The versions of libpng and freetype on my system don't have development headers, and I haven't had the time to make sure that I can rebuild them without affecting any other packages.) I can push a rebased version, but I won't actually be able to test anything on my current setup. Any chance someone can test a rebased PR and make any necessary changes?

@ChrisBeaumont Thanks for the checking on the tests. Before cleaning out my system, I tried to make sure the tests were discoverable without simply moving it to the test directory, but maybe I should just go ahead and move the test...

@adrn
Contributor
adrn commented Sep 17, 2013

Am not sure the URL functionality is working in this?

Also, I moved the tests to the correct spot -- see the PR I just submitted to your branch.

@pelson pelson commented on an outdated diff Sep 17, 2013
lib/matplotlib/__init__.py
@@ -917,21 +948,43 @@ def rc_params_from_file(fname, fail_on_error=False):
http://matplotlib.sf.net/_static/matplotlibrc or from the matplotlib source
distribution""" % (key, cnt, fname), file=sys.stderr)
- if ret['datapath'] is None:
- ret['datapath'] = get_data_path()
+ return config
+
+
+def rc_params_from_file(fname, fail_on_error=False):
@pelson
pelson Sep 17, 2013 Member

The difference between rc_params_from_file and rc_params_in_file are so minimal I'd be tempted to factor this into a keyword - something like use_default_template=True/False?

@pelson
Member
pelson commented Sep 17, 2013

I'm late to this party, but I think this is a really good change. 👍

I'm not the biggest fan of the rcParams system to control styling (which is the current state) from a configuration file - instead I agree with many of the other opinions proffered here that the style should be separate from the actual configuration.

I don't think that should be bundled into this PR, but I do think that we should look at ensuring that style is defined in the source code (explicitly linking to "style files" is fine) rather than allowing "global" default style changes, ideally before a v1.4 release...

Nice stuff @tonysyu!

@mdboom
Member
mdboom commented Sep 17, 2013

Yeah -- I completely concur with @pelson. And also that we shouldn't let that hold up this specific PR.

@pelson
Member
pelson commented Sep 18, 2013

I don't think style is documented in the API docs. Would you mind adding the appropriate file. I'm also keen to reduce the number of functions in lib/matplotlib/__init__.py so a keyword would be valuable. Other than that I'm 👍

@pelson
Member
pelson commented Sep 18, 2013

There has been a lot of great feedback on this PR so I'd like to get a roll call from > 50% that they are +1 before we merge this (any one else please feel free to chime in with your 👍 / 👎 .

@ChrisBeaumont
Contributor

I think @adrn Has a PR against @tonysyu's repo that adds support for gists. That could be considered here, or in a new PR. I'm fine with either

@pelson
Member
pelson commented Sep 18, 2013

or in a new PR

Definately. Love the gist idea though. Can I put you down as a 👍 @ChrisBeaumont?

@WeatherGod WeatherGod commented on an outdated diff Sep 18, 2013
lib/matplotlib/__init__.py
@@ -856,21 +862,45 @@ def rc_params(fail_on_error=False):
return rc_params_from_file(fname, fail_on_error)
-def rc_params_from_file(fname, fail_on_error=False):
- """Return a :class:`matplotlib.RcParams` instance from the
- contents of the given filename.
+URL_REGEX = re.compile(r'http://|https://|ftp://|file://|file:\\')
+
+
+def is_url(filename):
+ """Return True if string is an http or ftp path."""
@WeatherGod
WeatherGod Sep 18, 2013 Member

This docstring leaves out the possibility of file:// protocall.

@WeatherGod WeatherGod and 1 other commented on an outdated diff Sep 18, 2013
lib/matplotlib/__init__.py
+URL_REGEX = re.compile(r'http://|https://|ftp://|file://|file:\\')
+
+
+def is_url(filename):
+ """Return True if string is an http or ftp path."""
+ return URL_REGEX.match(filename) is not None
+
+
+@contextlib.contextmanager
+def _open_file_or_url(fname):
+ if is_url(fname):
+ f = urlopen(fname)
+ yield f
+ f.close()
+ else:
+ with open(fname) as f:
@WeatherGod
WeatherGod Sep 18, 2013 Member

Just wondering, would there be any newline issues if someone creates a style file on windows and it is used on a linux machine?

@mdboom
mdboom Sep 18, 2013 Member

The file is opened in text mode, so it should do universal newline handling.

@ChrisBeaumont
Contributor

Yes +1. Thanks @tonysyu, this is great

@mdboom
Member
mdboom commented Sep 18, 2013

👍 from me, modulo addressing @WeatherGod's comments -- I'm fine if @adrn's gist thing doesn't go in before merging this -- but I wouldn't want it to get lost.

@WeatherGod WeatherGod commented on an outdated diff Sep 18, 2013
lib/matplotlib/style/core.py
+def is_style_file(filename):
+ """Return True if the filename looks like a style file."""
+ return STYLE_FILE_PATTERN.match(filename) is not None
+
+
+def use(name):
+ """Use matplotlib style settings from a known style sheet or from a file.
+
+ Parameters
+ ----------
+ name : str or list of str
+ Name of style or path/URL to a style file. For a list of available
+ style names, see `style.available`. If given a list, each style is
+ applied from first to last in the list.
+ """
+ if np.isscalar(name):
@WeatherGod
WeatherGod Sep 18, 2013 Member

we have cbook.is_scalar()

@pelson
Member
pelson commented Sep 18, 2013

@tonysyu - if you could address @WeatherGod and my own outstanding comments I think we can get this merged in the next 24hrs.

@WeatherGod
Member

From an organizational perspective, I don't see why the tests are off into their own location. I would have thought to find the tests with all the other tests. I am also not a fan of core.py, but that is just a personal preference.

As for a few more things that are definitely needed: Add a "what's new" entry. Also, I would like to see an actual page describing these "Style Sheets" in the user's guide. Right now, we just have a couple of examples and the auto-generated API docs.

@mdboom
Member
mdboom commented Sep 18, 2013

@WeatherGod: I had missed that the tests were over there. It's not bad organizationally, but given the way our test harness works, they won't be run. They should be moved to matplotlib/tests/test_style.py and an entry added for them in __init__.py:default_test_modules.

@adrn
Contributor
adrn commented Sep 18, 2013

FYI In my PR on @tonysyu's branch, I moved the tests...

@pelson
Member
pelson commented Sep 18, 2013

FYI In my PR on @tonysyu's branch, I moved the tests...

😄 I like it.

@tacaswell
Member

Looks good to me, but I just skimmed the code.

My main concern as that the current default style does not change without ample warning and there is an easy way to roll back to the old style (which this provides) if it does.

@tonysyu
Contributor
tonysyu commented Sep 19, 2013

Hi all: Thanks for the renewed interest in this PR. I made a few code updates as suggested here, but I've conspicuously left out updates for the API docs and "what's new". Unfortunately, I'll be quite busy for the next couple of weeks, so I won't be able to work on those right now, but I'm happy to pick it back up later (either here or in a separate PR). That said, I certainly wouldn't complain if someone wanted to hijack this PR and write those docs for me ;)

@pelson
Member
pelson commented Sep 19, 2013

@adrn - the gauntlet has been laid down - do you want to pick-up the baton with your own PR extending this work and submit a PR to matplotlib master?

@adrn
Contributor
adrn commented Sep 20, 2013

I'll give this a go this weekend and see where I get.

@tacaswell
Member

To make this more confusing I wanted to use this (it took me a while to figure out rc_from_file pulls in the default) and ended up with my own branch that is re-based on current master and merges in @adrn 's work (but smashed down into one commit, sorry) a naming conflict resolution and some house keeping.

Lives at https://github.com/tacaswell/matplotlib/tree/stylesheets

@tonysyu
Contributor
tonysyu commented Sep 28, 2013

@tacaswell Thanks for the fixes. Would you like to open up a PR against my branch. I should have some time to finish this up now.

I'm still a bit iffy about requiring an extension for local files but not for remote files, but if that really simplifies URL support, then I'm fine with it.

@tacaswell
Member

There is a PR against your branch from a branch where I cherry-picked the 4 commits over.

I think a PR from my first branch would be very messy because I re-based on-to master.

The changes from @adrn I think make it so you don't need an extension any where (experimentally it is the case that you don't need an extension).

@tonysyu
Contributor
tonysyu commented Sep 29, 2013

Merged changes by @tacaswell and @adrn. Also added entries to User Guide and What's New page. Anything else.

@tonysyu
Contributor
tonysyu commented Sep 29, 2013

Note: I rebased because I thought there were changes in master that broke my doc build. Turns out that I had some
bit-rotted files that were breaking the build. If you checked out this branch (and want to test some more), you'll have to do forced pull.

@mdboom mdboom commented on the diff Sep 30, 2013
doc/users/whats_new.rst
@@ -53,6 +53,25 @@ Controls whether figures are saved with a transparent
background by default. Previously `savefig` always defaulted
to a non-transparent background.
+
+``style`` package added
+```````````````````````
+You can now easily switch between different styles using the new ``style``
+package::
+
+ >>> from matplotlib import style
+ >>> style.use('dark_background')
@mdboom
mdboom Sep 30, 2013 Member

It would be nice to have an inline plot here. Doesn't have to be done before merge, just a note to self, really.

@mdboom
Member
mdboom commented Sep 30, 2013

I think this is ready to merge, but I'm going to get to the bottom of the test failures first.

@adrn
Contributor
adrn commented Sep 30, 2013

Thanks for taking care of this @tacaswell

@mdboom
Member
mdboom commented Sep 30, 2013

I've filed a PR against this that should hopefully fix the issues with the tests having styles leaking from one test to the next.

@mdboom
Member
mdboom commented Sep 30, 2013

Now something is really broken on Python 3, if anyone wants to have a look.

@tonysyu
Contributor
tonysyu commented Oct 17, 2013

Unlikely, but I'm going to close in reopen in case there was somehow an error in the Travis CI build that caused the two Python 3 builds to fail at the same time. ("Have you tried turning it off and back on again?")

@tonysyu tonysyu closed this Oct 17, 2013
@tonysyu tonysyu reopened this Oct 17, 2013
@tonysyu
Contributor
tonysyu commented Oct 17, 2013

Note: Rebased on master and forced a push. If you've pulled this down as a local branches, you'll have to do a clean (or forced) pull.

@pelson pelson commented on the diff Oct 18, 2013
lib/matplotlib/pyplot.py
@@ -25,6 +25,7 @@
import matplotlib
import matplotlib.colorbar
+from matplotlib import style
@pelson
pelson Oct 18, 2013 Member

I think we should start a block of "guaranteed imports" within the pyplot namespace, otherwise this is liable to get nuked with an over-eager pyflakes (sure, there are test which catch this, but it does no harm to be explicit here).

@pelson pelson commented on an outdated diff Oct 18, 2013
lib/matplotlib/style/__init__.py
@@ -0,0 +1 @@
+from core import use, context, available, library, reload_library
@pelson
pelson Oct 18, 2013 Member

I love the fact you've used a folder here. sub-packages are one honking great idea -- let's do more of those!

@pelson
pelson Oct 18, 2013 Member

Obviously this comment only makes sense I've you've gone through the matplotlib code in as much detail as I have and you dislike having to scroll through the tens and tens of ".py" files in the top level matplotlib package.

@pelson
Member
pelson commented Oct 18, 2013

Once the tests pass, I'm happy for this to go in. The one word I'd like to see in the "what's new" is "experimental" as I think we should accept that as users make use of this, we may want to change interfaces etc.
Other than that, I can see that this will be a very powerful and useful feature and we should get it merged asap.

Thanks @tonysyu!

@tacaswell
Member

@tonysyu it looks like you lost mbdoom's patch to fix style-leakage when you re-based.

@tonysyu
Contributor
tonysyu commented Oct 19, 2013

@tacaswell Thanks for catching the dropped commits.

@pelson Thanks for the review. I added a note about the experimental nature of this feature. Unfortunately, I have no idea what's causing the test failures on the Python 3 buildbot.

@tacaswell
Member

It looks like there was a major issue building the test-suite in python 3. Is there a way to re-trigger this build?

@mdboom
Member
mdboom commented Oct 30, 2013

I don't know of a way to retrigger the build other than doing a rebase (i.e. something that would change the commit hash).

EDIT: Correction -- if I sign in to Travis, I can restart the build, and I've done so.

@tacaswell
Member

Sorry, I sent a PR to @tonysyu but didn't leave a note here. The issue with this PR is that some of the new files escaped the mass from __future__ ... import + six change over and there were some print blah statements.

@tonysyu tonysyu closed this Nov 13, 2013
@tonysyu tonysyu reopened this Nov 13, 2013
@tacaswell
Member

@tonysyu Needs a rebase, it doesn't merge cleanly right now.

@tonysyu
Contributor
tonysyu commented Nov 14, 2013

Latest commit is just a sanity check on the Travis CI test failures. Do not merge until it's reverted.

@tonysyu
Contributor
tonysyu commented Nov 14, 2013

Surprisingly, the Travis CI test failures for Python 3 have nothing to do with the tests added for this PR, as demonstrated by the failure of tonysyu@124cdfc.

Any ideas?

tonysyu and others added some commits Jul 21, 2013
@tonysyu tonysyu Add easy style sheet selection. 643c74b
@tonysyu tonysyu Add rc_params_in_file to return partially-filled RcParams
This allows style sheets to update the current settings instead of overwriting them.
3270aa4
@tonysyu tonysyu Rename style files to `*.style` d83a03c
@tonysyu tonysyu Allow style.use to open URLs c8cc486
@tonysyu tonysyu Remove pyplot import 455b54c
@tonysyu tonysyu Add style context manager and tests 7769b29
@tonysyu tonysyu Change style extension to *.mplstyle 3914089
@tonysyu tonysyu Got a little crazy with the whitespace c3fae2e
@tonysyu tonysyu Add style package and data to setupext.py a3de231
@tonysyu tonysyu Move test so that it actually runs. d56f73e
@tonysyu tonysyu Use explicit string check ec6ce6b
@tonysyu tonysyu Hide rc_params_in_file from parent namespace 5fdc037
@adrn @tonysyu adrn fix url rc specification
smoothed over conflicts

add test for URL stylesheet

whoops, remove extraneous regex I added...

move test to proper location

arg, don't need urllib2 either...didn't realize functionality was in core mpl
ea63c99
@tonysyu tonysyu Remove usage of import * 0c7437c
@tonysyu tonysyu Clarify docstring 200d2e0
@tacaswell @tonysyu tacaswell added `matplotlib.style` to pyplot import list 7392ce6
@tacaswell @tonysyu tacaswell fixed divergent naming scheme eaa23ee
@tacaswell @tonysyu tacaswell pep8 clean up 5f80ca1
@tonysyu tonysyu Add docs for style package f5ecf5e
@tonysyu tonysyu Import style package for easy use. a8ef5bf
@tonysyu tonysyu Use style package from pyplot dc291e0
@tonysyu tonysyu Fix test c5b5bb4
@mdboom @tonysyu mdboom Clear style settings between tests
Conflicts:
	lib/matplotlib/tests/test_style.py

Conflicts:
	lib/matplotlib/tests/test_style.py
46a725b
@tacaswell @tonysyu tacaswell pep8 7ac26ee
@tonysyu tonysyu Add note that style sheets are experimental. d372a35
@tacaswell @tonysyu tacaswell added python3 emulation code + six + fixed up print calls e714c77
@tacaswell @tonysyu tacaswell removed unneeded print statements
removed some of the unneeded imports
512b77c
@tonysyu tonysyu Attempt to fix python 3 test errors on Travis CI 17282c8
@tonysyu tonysyu Remove test from list to test Travis CI failure. a6142fc
@tonysyu tonysyu Remove test file to test Travis CI failure 19e7bed
@tonysyu tonysyu Revert commits used to test Travis CI test failures.
End result of the experiment: The failure isn't related to the tests
added by this PR.
c604498
@tonysyu tonysyu Fix import for python 3 0b098e2
@tonysyu tonysyu Use iteritems from `six` module 7e2bffb
@tonysyu tonysyu Add compatibility layer for Python 3's urlopen 1d87f34
@tonysyu
Contributor
tonysyu commented Nov 17, 2013

Tests are almost passing. Maybe Python 2.6 has an issue with a context-manager returning a generator?

@mdboom
Member
mdboom commented Nov 18, 2013

I've made a PR against this one with a Python 2.6 fix

@tonysyu
Contributor
tonysyu commented Nov 18, 2013

Woot! Tests passing!

@mdboom mdboom merged commit 0e7daad into matplotlib:master Nov 18, 2013

1 check passed

default The Travis CI build passed
Details
@mdboom
Member
mdboom commented Nov 18, 2013

Merged. Thanks, Tony. This was a big one!

@ChrisBeaumont
Contributor

Phew! That was harder than I expected, but I'm glad this is now part of MPL

@tonysyu
Contributor
tonysyu commented Nov 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 :).

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