Skip to content

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

Merged
efiring merged 6 commits into
matplotlib:masterfrom
mdboom:blacklist-style-params
Dec 31, 2015
Merged

efiring merged 6 commits into
matplotlib:masterfrom
mdboom:blacklist-style-params

Conversation

@mdboom

@mdboom mdboom commented Nov 8, 2015

Copy link
Copy Markdown
Member

Needs a what's new

@mdboom mdboom added this to the next major release (2.0) milestone Nov 8, 2015
Comment thread lib/matplotlib/style/core.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing a leading underscore.

@tacaswell

Copy link
Copy Markdown
Member

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

@mdboom

mdboom commented Nov 12, 2015

Copy link
Copy Markdown
Member Author

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

@mdboom

mdboom commented Nov 17, 2015

Copy link
Copy Markdown
Member Author

I think this one is good to go.

Comment thread lib/matplotlib/style/core.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. That makes sense.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@WeatherGod

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

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

mdboom commented Nov 17, 2015

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member

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

mdboom commented Nov 17, 2015

Copy link
Copy Markdown
Member Author

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

mdboom commented Nov 20, 2015

Copy link
Copy Markdown
Member Author

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

@tacaswell

Copy link
Copy Markdown
Member

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

mdboom commented Dec 6, 2015

Copy link
Copy Markdown
Member Author

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

mdboom commented Dec 29, 2015

Copy link
Copy Markdown
Member Author

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

@efiring

efiring commented Dec 30, 2015

Copy link
Copy Markdown
Member

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

efiring added a commit that referenced this pull request Dec 31, 2015
Fix #4855: Blacklist rcParams that aren't style
@efiring
efiring merged commit 8decc8c into matplotlib:master Dec 31, 2015
@efiring

efiring commented Dec 31, 2015

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

Should this be backported to 2.0?

@mdboom

mdboom commented Jan 1, 2016

Copy link
Copy Markdown
Member Author

yes.

jenshnielsen pushed a commit to jenshnielsen/matplotlib that referenced this pull request Jan 5, 2016
@jenshnielsen jenshnielsen mentioned this pull request Jan 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants