Fix #4855: Blacklist rcParams that aren't style #5440

mdboom · Nov 8, 2015

~~Needs a what's new~~

Missing a leading underscore.

The name is not so clear in indicating that a dict of valid params will be returned; how about _screened_style_params or _valid_style_params?

Would it make sense to raise a warning if a a blacklisted key is found?

tacaswell · Nov 11, 2015

Should also remove all of the black-listed entries from classic

mdboom · Nov 12, 2015

@tacaswell: Good point about removing blacklisted entries from the classic style. Done.

mdboom · Nov 17, 2015

I think this one is good to go.

There is a bit of a disconnect here. rcdefaults() is more than just style stuff, but it doesn't make sense to blacklist entries there, either, does it?

@mdboom, I think @WeatherGod has a good point here: 'default' style should modify only non-blacklisted things, shouldn't it?

I see. That makes sense.

Fixed.

what about the default keymappings?

In that case, I thought using style might be handy, e.g. an Emacs key style (not that one could do that given our current framework, but you get the idea).

WeatherGod · Nov 17, 2015

I think this raises a bit more general question of what is the line between styles and settings. There are tons of other settings such as the default keymappings, animation settings, and such that may or may not make sense as a "style". I am fine with what has been chosen so far, it is just a question of how much more do we want to blacklist.

WeatherGod · Nov 17, 2015

Also, do we know if people have been using style.use() as a convenient way to apply custom rcParams? If so, then this change could break that use-case (not that it would have been a good thing to do in the first place, though)

mdboom · Nov 17, 2015

Also, do we know if people have been using style.use() as a convenient way to apply custom rcParams?

No, but I know of at least three cases where style was used to turn interactive on and caused end users no end of trouble... ;)

WeatherGod · Nov 17, 2015

This is all fine, but we still don't have a definition that can help guide us in choosing what should and shouldn't be allowed in a style file. So, to get the conversation rolling, I'll propose a definition: "a non-style parameter is one whose change would have no effect after importing pyplot". This definition might not be good enough. Should the line be drawn at figure creation? axes creation?

mdboom · Nov 17, 2015

My definition is things that affect the appearance of the plot are style. Things that are related to the environment/platform in use and other technical considerations are not.

mdboom · Nov 20, 2015

@tacaswell: Any thoughts on the definition of "style parameters" here?

tacaswell · Nov 20, 2015

I would say it has to have a visual effect, rather than a ui effect (interactive, key bindings, default file types) which i think lines up with what @mdboom said and is a super set of @WeatherGod. Things like dpi are on the edge and should probably be allowed.

mdboom · Dec 6, 2015

I've added a comment about what should be blacklisted. I hope that addresses the comments above, and otherwise this is good-to-merge.

mdboom · Dec 29, 2015

I think this is ready-to-merge, unless there are other comments.

efiring · Dec 30, 2015

@mdboom, see @WeatherGod's first inline comment.

efiring · Dec 31, 2015

I have the impression that some might want tighter restrictions--a larger blacklist. Merging this does not preclude such changes, but it seems worthwhile to get the mechanism in place without further delay.

jenshnielsen · Jan 1, 2016

Should this be backported to 2.0?

mdboom · Jan 1, 2016

yes.

mdboom Fix #4855: Blacklist rcParams that aren't style

c90bcfb

mdboom added the needs_review label Nov 8, 2015

mdboom added this to the next major release (2.0) milestone Nov 8, 2015

mdboom added some commits Nov 9, 2015

mdboom Fix, per comments in PR

22f99bb

mdboom Fix

fdae5de

mdboom Remove blacklisted styles from classic style

e68d239

mdboom Add what's new

0946c1c

efiring added a commit that referenced this pull request Dec 31, 2015

efiring Merge pull request #5440 from mdboom/blacklist-style-params
Fix #4855: Blacklist rcParams that aren't style
8decc8c

efiring removed the needs_review label Dec 31, 2015

jenshnielsen referenced this pull request Jan 1, 2016
Closed
Using default style raises warnings about non style parameters #5777

jenshnielsen added a commit to jenshnielsen/matplotlib that referenced this pull request Jan 5, 2016

efiring + jenshnielsen Merge pull request #5440 from mdboom/blacklist-style-params
Fix #4855: Blacklist rcParams that aren't style
4c319fc

jenshnielsen referenced this pull request Jan 5, 2016
Merged
Backport blacklist #5796

      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 _apply_style(d):
+    +    mpl.rcParams.use(blacklist_style_params(d))

@@ -34,11 +34,27 @@
      STYLE_FILE_PATTERN = re.compile('([\S]+).%s$' % STYLE_EXTENSION)
+    +# A list of rcParams that should not be applied from styles
+    +STYLE_BLACKLIST = set([
+    +    'interactive', 'backend', 'backend.qt4', 'webagg.port',
+    +    'webagg.port_retries', 'webagg.open_in_browser', 'backend_fallback',
+    +    'toolbar', 'timezone', 'datapath', 'figure.max_open_warning',
+    +    'savefig.directory', 'tk.window_focus', 'hardcopy.docstring'])
+    +
+    +
+    +def _blacklist_style_params(d):

@@ -34,11 +34,27 @@
      STYLE_FILE_PATTERN = re.compile('([\S]+).%s$' % STYLE_EXTENSION)
+    +# A list of rcParams that should not be applied from styles
+    +STYLE_BLACKLIST = set([
+    +    'interactive', 'backend', 'backend.qt4', 'webagg.port',
+    +    'webagg.port_retries', 'webagg.open_in_browser', 'backend_fallback',
+    +    'toolbar', 'timezone', 'datapath', 'figure.max_open_warning',
+    +    'savefig.directory', 'tk.window_focus', 'hardcopy.docstring'])
+    +
+    +
+    +def _blacklist_style_params(d):
+    +    return dict((k, v) for (k, v) in d.items() if k not in STYLE_BLACKLIST)

@@ -71,18 +96,18 @@ def use(style):
          for style in styles:
              if not cbook.is_string_like(style):
-    -            mpl.rcParams.update(style)
+    +            _apply_style(style)
                  continue
              elif style == 'default':
                  mpl.rcdefaults()

@@ -34,11 +35,35 @@
      STYLE_FILE_PATTERN = re.compile('([\S]+).%s$' % STYLE_EXTENSION)
+    +# A list of rcParams that should not be applied from styles
+    +STYLE_BLACKLIST = set([
+    +    'interactive', 'backend', 'backend.qt4', 'webagg.port',
+    +    'webagg.port_retries', 'webagg.open_in_browser', 'backend_fallback',
+    +    'toolbar', 'timezone', 'datapath', 'figure.max_open_warning',
+    +    'savefig.directory', 'tk.window_focus', 'hardcopy.docstring'])

matplotlib/matplotlib

Fix #4855: Blacklist rcParams that aren't style #5440

mdboom commented Nov 8, 2015

mdboom added the needs_review label Nov 8, 2015

mdboom added this to the next major release (2.0) milestone Nov 8, 2015

efiring commented on an outdated diff Nov 8, 2015

efiring commented on an outdated diff Nov 8, 2015

efiring commented on an outdated diff Nov 8, 2015

mdboom added some commits Nov 9, 2015

tacaswell commented Nov 11, 2015

mdboom commented Nov 12, 2015

mdboom commented Nov 17, 2015

WeatherGod and 2 others commented on an outdated diff Nov 17, 2015

WeatherGod commented on the diff Nov 17, 2015

WeatherGod commented Nov 17, 2015

WeatherGod commented Nov 17, 2015

mdboom commented Nov 17, 2015

WeatherGod commented Nov 17, 2015

mdboom commented Nov 17, 2015

mdboom commented Nov 20, 2015

tacaswell commented Nov 20, 2015

mdboom commented Dec 6, 2015

mdboom commented Dec 29, 2015

efiring commented Dec 30, 2015

efiring added a commit that referenced this pull request Dec 31, 2015

efiring merged commit `8decc8c` into matplotlib:master Dec 31, 2015
3 checks passed

3 checks passed

efiring removed the needs_review label Dec 31, 2015

efiring commented Dec 31, 2015

jenshnielsen referenced this pull request Jan 1, 2016

Using default style raises warnings about non style parameters #5777

jenshnielsen commented Jan 1, 2016

mdboom commented Jan 1, 2016

jenshnielsen added a commit to jenshnielsen/matplotlib that referenced this pull request Jan 5, 2016

jenshnielsen referenced this pull request Jan 5, 2016

Backport blacklist #5796

matplotlib/matplotlib

Join GitHub today

Fix #4855: Blacklist rcParams that aren't style #5440

Conversation

mdboom commented Nov 8, 2015

mdboom added the needs_review label Nov 8, 2015

mdboom added this to the next major release (2.0) milestone Nov 8, 2015

Show 1 comment Hide comment efiring commented on an outdated diff Nov 8, 2015

Show 1 comment Hide comment efiring commented on an outdated diff Nov 8, 2015

Show 1 comment Hide comment efiring commented on an outdated diff Nov 8, 2015

mdboom added some commits Nov 9, 2015

tacaswell commented Nov 11, 2015

mdboom commented Nov 12, 2015

mdboom commented Nov 17, 2015

Show 4 comments Hide comments WeatherGod and 2 others commented on an outdated diff Nov 17, 2015

WeatherGod commented on the diff Nov 17, 2015

WeatherGod commented Nov 17, 2015

WeatherGod commented Nov 17, 2015

mdboom commented Nov 17, 2015

WeatherGod commented Nov 17, 2015

mdboom commented Nov 17, 2015

mdboom commented Nov 20, 2015

tacaswell commented Nov 20, 2015

mdboom commented Dec 6, 2015

mdboom commented Dec 29, 2015

efiring commented Dec 30, 2015

All checks have passed

efiring added a commit that referenced this pull request Dec 31, 2015

Hide details View details efiring merged commit 8decc8c into matplotlib:master Dec 31, 2015 3 checks passed

3 checks passed

efiring removed the needs_review label Dec 31, 2015

efiring commented Dec 31, 2015

jenshnielsen referenced this pull request Jan 1, 2016

jenshnielsen commented Jan 1, 2016

mdboom commented Jan 1, 2016

jenshnielsen added a commit to jenshnielsen/matplotlib that referenced this pull request Jan 5, 2016

jenshnielsen referenced this pull request Jan 5, 2016

efiring commented on an outdated diff Nov 8, 2015

efiring commented on an outdated diff Nov 8, 2015

efiring commented on an outdated diff Nov 8, 2015

WeatherGod and 2 others commented on an outdated diff Nov 17, 2015

efiring merged commit `8decc8c` into matplotlib:master Dec 31, 2015
3 checks passed