Style RC parameter #4240

Mrngilles · 2015-03-18T10:13:11Z

Added a style RC parameter to be able to import styles from stylesheet or matplotlibrc.
You can call a parent stylesheet using the following syntax in your matplotlibrc or a stylesheet:
'style : chosen_style' if the style is in the library (built-in styles or a style put in your user config path
'style : path/to/style' to get a style from anywhere.

I also added a test to make sure we get the good rcParameters when calling parent styles. For that, I added sample stylesheets in the sample_data parameter, that form the same tree as the one seen in the get_style_parameters function. It should cover every possible override possible.

Priority order remark : When calling styles this way, the first ones will have the highest priority.

Todo:

Breakdown in smaller functions to follow the flow more easily

Mrngilles · 2015-03-18T13:26:58Z

I get errors from travis that my stylesheets cannot be found... I am not sure how I could get it to work. Also, it is now in the sample_data folder, which I think is not the best location for data that will be used only for tests.
Do you have any idea how I could solve this?

Edit: It seems my folder won't install with python setup.py install. Do you have any idea why?

Edit 2: Tried and find how was imported the tests/baseline_images data for the tests, and found it out in the setupext.py and test_only.py files. Tried adding my styles folder here and put it in the tests folder.

tonysyu · 2015-03-19T04:18:44Z

Thanks for breaking this out into its own PR!

A general comment about the tests and sample style files: I think all of the style files are unnecessary. Having tests that depend on values that are in a separate file makes the logic of the tests hard to follow. Instead, I would suggest reusing the temp_style context manager to create a couple of style files. (BTW, I noticed that I neglected to properly connect the settings argument of temp_style; there should have been an if settings is None before using the default.) That way, the test inputs and outputs are clear.

tonysyu · 2015-03-19T04:24:04Z

lib/matplotlib/rcsetup.py

@@ -501,6 +501,8 @@ def __call__(self, s):

 # a map from key -> value, converter
 defaultParams = {
+    'style': ['', validate_stringlist],


As mentioned in the original PR, I don't think it's important to support a list of parent stylesheets---at least not initially. I could be convinced otherwise, but I think it adds complication to the logic.

Actually, getting a list of parent stylesheets is kind of my main idea. This way, you could have really small stylesheets setting only a part of the plot (e.g. only the font, or the lines width) that could be associated to create easily bigger stylesheets.

Personally, I see two broad categories for style sheets: Those focused on aesthetics and those focused on layout (for slides, journal papers, etc.). I think these should be combined at runtime with style.use. I don't see the need for more fine grained partitioning, but I imagine others would. I don't want to push too much, but I think a simpler initial implementation is a good idea.

On the top on being able to create stylesheets by "cascading" them (it's a very small implementation of cascading...), you could have a style in your matplotlibrc so that you don't have to call the style.use function every time you create a new script.

I think you're right, actually: Combining multiple stylesheets in your default matplotlibrc file will probably be a common use case.

That said, I think the implementation might be a bit clearer with a different approach. Have you thought of writing this using recursive dictionary updates instead of looping over parent styles? I've played around with an alternate implementation if you're open to the idea.

Can you explain a bit more on detail about the recursive dictionary update? I'm willing to do the best implementation possible for this, so if this can be cleaner...

Envoyé depuis mon appareil Samsung

-------- Message d'origine --------
De : Tony S Yu notifications@github.com
Date : 22/06/2015 06:58 (GMT+01:00)
À : matplotlib/matplotlib matplotlib@noreply.github.com
Cc : Gilles Marin gillesmarin1706@gmail.com
Objet : Re: [matplotlib] Style RC parameter (#4240)

In lib/matplotlib/rcsetup.py:

@@ -501,6 +501,8 @@ def call(self, s):

a map from key -> value, converter

defaultParams = {

'style': ['', validate_stringlist],

I think you're right, actually: Combining multiple stylesheets in your default matplotlibrc file will probably be a common use case.

That said, I think the implementation might be a bit clearer with a different approach. Have you thought of writing this using recursive dictionary updates instead of looping over parent styles? I've played around with an alternate implementation if you're open to the idea.

—
Reply to this email directly or view it on GitHub.

Here's a gist that flattens a dictionary containing a special key pointing to "parent" dictionaries: https://gist.github.com/tonysyu/b02be7df210c4369afa6. That's a generic implementation with nothing specific to matplotlib styles. To use it in this case you could write something like the following:

PARENT_STYLES = 'styles' flattened_style = flatten_style_dict({PARENT_STYLES: styles}) def flatten_style_dict(styles_dict): return flatten_inheritance_dict(styles_dict, PARENT_STYLES, expand_parent=_expand_parent) def _expand_parent(parent_style): if cbook.is_string_like(parent_style): return get_style_parameters(parent_style) return parent_style

To actually integrate this with the style module, you'd want to remove the loop from use and just call update on the flattened_style dictionary. Note that the current tests won't pass with this function because the order of styles in the PARENT_STYLES is flipped. If you flip the order of parent styles in your mplstyle files, the test will pass.

tacaswell · 2015-06-17T04:52:48Z

Re-milestoned this as 'proposed next point release' which mean it will not be a blocker for the next point release.

This looks like it still has a good deal of work?

WeatherGod · 2015-07-12T13:30:05Z

@tacaswell , @Mrngilles , are there any outstanding issues here?

Mrngilles · 2015-07-12T17:05:49Z

@WeatherGod
There should not be. This should work, but could be improved. I will have to, and will as soon as possible.

TODO :

Tests reduction
Look in more details into the dict flattening as suggested by @tonysyu

WeatherGod · 2015-07-13T13:49:49Z

Given that we want to do a feature freeze (today, preferably), do you have
any objections to merging this now as-is (note, I have not personally
reviewed this PR)

On Sun, Jul 12, 2015 at 1:05 PM, Gilles Marin notifications@github.com
wrote:

There should not be. This should work, but could be improved. I will have
to, and will as soon as possible.

TODO :

Tests reduction

Look in more details into the dict flattening as suggested by
@tonysyu https://github.com/tonysyu

—
Reply to this email directly or view it on GitHub
#4240 (comment)
.

Mrngilles · 2015-07-13T13:54:04Z

I have no objection, as this should be working.

On July 13, 2015 3:50:00 PM GMT+02:00, Benjamin Root notifications@github.com wrote:

Given that we want to do a feature freeze (today, preferably), do you
have
any objections to merging this now as-is (note, I have not personally
reviewed this PR)

On Sun, Jul 12, 2015 at 1:05 PM, Gilles Marin
notifications@github.com
wrote:

There should not be. This should work, but could be improved. I will
have
to, and will as soon as possible.

TODO :

Tests reduction

Look in more details into the dict flattening as suggested by
@tonysyu https://github.com/tonysyu

—
Reply to this email directly or view it on GitHub

#4240 (comment)
.

Reply to this email directly or view it on GitHub:
#4240 (comment)

Envoyé de mon appareil Android avec K-9 Mail. Veuillez excuser ma brièveté.

tacaswell · 2015-07-21T17:27:38Z

That very helpful error message means 'nose could not import the module matplotlib.tests.test_style but silently snarfed the exception and are now telling you that the explicitly white listed modules to test do not exist'.

Are you using assert_raises as a context manager?

tacaswell · 2015-07-21T17:28:00Z

I kicked the 2.7 and 3.3 tests, they look like timeout issues we intermittently have with latex

Mrngilles · 2015-07-21T17:54:10Z

Indeed the tests for the dictionary flattening function use assert_raise as a context manager. Do you think changing that will make it work?
Edit: I found this link to a patch correcting the context manager use of assert_raises so I guess even if it is not a problem anymore (which seems it is), I was at some point. Does using assert_raises the basic way (assert_raises(Exception, function, *args)) solve this issue?

tacaswell · 2015-07-21T18:43:51Z

Rather than iterating with travis, it is probably easiest to install a 2.6 enviroment with conda and see what is failing on import.

WeatherGod · 2015-07-21T20:28:02Z

Yes, I got bitten by this once before. For some reason, assert_raise() is
not available as a context manager in python 2.6.

On Tue, Jul 21, 2015 at 2:43 PM, Thomas A Caswell notifications@github.com
wrote:

Rather than iterating with travis, it is probably easiest to install a 2.6
enviroment with conda and see what is failing on import.

—
Reply to this email directly or view it on GitHub
#4240 (comment)
.

Mrngilles · 2015-07-21T20:29:52Z

Maybe that was a problem, but it was not the only one. assert_is_not and assert_is_instance were not in nose in python 2.6, so could not be imported.

tacaswell · 2015-08-30T22:39:24Z

This needs a re-base again.

Mrngilles · 2015-10-03T16:16:22Z

I am going to rebase this against v1.5.x. Should I also create a new PR against v1.5.x or can I leave it against master?

tacaswell · 2015-10-03T16:55:49Z

Leave it against master, we will cherry-pick the merge back to 1.5.x.

On Sat, Oct 3, 2015 at 12:16 PM Gilles Marin notifications@github.com
wrote:

I am going to rebase this against v1.5.x. Should I also create a new PR
against v1.5.x or can I leave it against master?

—
Reply to this email directly or view it on GitHub
#4240 (comment)
.

Mrngilles · 2016-03-18T09:41:20Z

Hi guys,
Sorry for the delay. I finally rebased my PR and squashed everything.
I think it's clearer this way.
Hope it'll do

tacaswell · 2016-03-18T19:22:50Z

lib/matplotlib/style/core.py

@@ -65,7 +66,7 @@ def _apply_style(d, warn=True):
    mpl.rcParams.update(_remove_blacklisted_style_params(d, warn=warn))


-def use(style):
+def use(styles):


bit concerned about the API break on changing the arg name.

I agree, I did not think about this.
Then, shoud I just adapt to keep the same API?

Yeah, just leave it as style and accept the some-what dodge plural issues.

@tonysyu

Allows to support cascading stylesheets directly from the matplorlibrc fixed an issue with depth > 1 added test getting parent styles + sample test styles fixed location of sample styles PEP8 fixes added import of stylesheets in setupext and test_only fixed stylesheet path in test_style other styles path fix other fix for styles path fixed an issue in style.use priority of styles passed added smaller functions for get_parents_styles function Fixed issues concerning parent styles importing other small functions to insert parent styles in the style list + small tweaks simplified the check of dictionnary like object using isinstance Few changes (some refactoring + docstring change) Using @tonysyu dictonnary flattening function to get children styles Added tests + dictionnary flattening fixes Docstring addition PEP8 fix pep8 fix removed style file imports needed for the now removed tests Fix for Py 2.6: some nose function did not exist

Mrngilles · 2016-12-08T18:51:31Z

New rebase on top of master.

Mrngilles · 2016-12-08T20:21:32Z

Not sure why the tests failed, but doesn't seem related to my rebase.

tacaswell · 2016-12-09T02:55:30Z

lib/matplotlib/style/core.py

+    return parent_style
+
+
+def flatten_inheritance_dict(child_dict, parent_key,


Why leave this public?

tacaswell · 2016-12-09T03:00:10Z

lib/matplotlib/style/core.py

+
+
+def flatten_inheritance_dict(child_dict, parent_key,
+                             expand_parent=lambda x: x):


and why leave this with a default value like this?

tacaswell · 2016-12-09T03:08:21Z

That test has been flaky recently.

There is some weird public vs private function things (there is a public function which is used in a private function which is called from a public function with is called from a private function which is used in use?). Not saying this is wrong, but it seems it could be simpler.

jklymak · 2018-10-06T01:17:50Z

This kind of got dropped. I'm also not 100% sure about the reason why someone would want this as an rcparam, nor has there been a user base clamouring for this. But please do re-open if I'm mistaken.

tacaswell added the status: needs review label Mar 18, 2015

tacaswell added this to the next point release milestone Mar 18, 2015

tonysyu reviewed Mar 19, 2015
View reviewed changes

tacaswell added status: needs revision and removed status: needs review labels Apr 13, 2015

tacaswell modified the milestones: proposed next point release, next point release Jun 17, 2015

tacaswell mentioned this pull request Jul 2, 2015

Add Seaborn style sheets; addresses #4566 #4570

Merged

Mrngilles force-pushed the style_rc branch from 8d75a75 to 35ab43a Compare February 27, 2016 14:40

tacaswell added status: needs review and removed status: needs revision labels Mar 18, 2016

tacaswell reviewed Mar 18, 2016
View reviewed changes

Marin Gilles and others added 2 commits December 8, 2016 18:05

API consistency Fix

105c80a

Mrngilles force-pushed the style_rc branch from b12fb0e to 105c80a Compare December 8, 2016 18:06

tacaswell reviewed Dec 9, 2016

View reviewed changes

anntzer added the topic: rcparams label Sep 16, 2017

tacaswell modified the milestones: 2.1 (next point release), 2.2 (next next feature release) Sep 24, 2017

jklymak added status: needs rebase and removed status: needs review labels May 24, 2018

jklymak closed this Oct 6, 2018

story645 removed this from the future releases milestone Oct 6, 2022

story645 mentioned this pull request Jul 10, 2023

Add xkcd style file. #14943

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Style RC parameter #4240

Style RC parameter #4240

Mrngilles commented Mar 18, 2015

Mrngilles commented Mar 18, 2015

tonysyu commented Mar 19, 2015

tonysyu Mar 19, 2015

Mrngilles Mar 19, 2015

tonysyu Mar 20, 2015

Mrngilles Jun 17, 2015

tonysyu Jun 22, 2015

Mrngilles Jun 22, 2015

tonysyu Jun 23, 2015

tacaswell commented Jun 17, 2015

WeatherGod commented Jul 12, 2015

Mrngilles commented Jul 12, 2015

WeatherGod commented Jul 13, 2015

Mrngilles commented Jul 13, 2015

tacaswell commented Jul 21, 2015

tacaswell commented Jul 21, 2015

Mrngilles commented Jul 21, 2015

tacaswell commented Jul 21, 2015

WeatherGod commented Jul 21, 2015

Mrngilles commented Jul 21, 2015

tacaswell commented Aug 30, 2015

Mrngilles commented Oct 3, 2015

tacaswell commented Oct 3, 2015

Mrngilles commented Mar 18, 2016

tacaswell Mar 18, 2016

Mrngilles Mar 21, 2016

tacaswell Mar 21, 2016

Mrngilles commented Dec 8, 2016

Mrngilles commented Dec 8, 2016

tacaswell Dec 9, 2016

tacaswell Dec 9, 2016

tacaswell commented Dec 9, 2016

jklymak commented Oct 6, 2018

		return parent_style


		def flatten_inheritance_dict(child_dict, parent_key,



		def flatten_inheritance_dict(child_dict, parent_key,
		expand_parent=lambda x: x):

Style RC parameter #4240

Style RC parameter #4240

Conversation

Mrngilles commented Mar 18, 2015

Mrngilles commented Mar 18, 2015

tonysyu commented Mar 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a map from key -> value, converter

Choose a reason for hiding this comment

tacaswell commented Jun 17, 2015

WeatherGod commented Jul 12, 2015

Mrngilles commented Jul 12, 2015

WeatherGod commented Jul 13, 2015

Mrngilles commented Jul 13, 2015

tacaswell commented Jul 21, 2015

tacaswell commented Jul 21, 2015

Mrngilles commented Jul 21, 2015

tacaswell commented Jul 21, 2015

WeatherGod commented Jul 21, 2015

Mrngilles commented Jul 21, 2015

tacaswell commented Aug 30, 2015

Mrngilles commented Oct 3, 2015

tacaswell commented Oct 3, 2015

Mrngilles commented Mar 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mrngilles commented Dec 8, 2016

Mrngilles commented Dec 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Dec 9, 2016

jklymak commented Oct 6, 2018