FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Make hatch linewidth an rcParam by mdboom · Pull Request #6198 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Make hatch linewidth an rcParam - #6198

Merged
tacaswell merged 4 commits into
matplotlib:masterfrom
mdboom:hatch-edge-width
Apr 11, 2016
ConversationCommits (4)ChecksFiles changed
Merged

Make hatch linewidth an rcParam#6198
tacaswell merged 4 commits into
matplotlib:masterfrom
mdboom:hatch-edge-width

Conversation

mdboom commented Mar 21, 2016

Copy link
Copy Markdown
Member

Fix #235

mdboom added this to the 2.0 (style change major release) milestone Mar 21, 2016
tacaswell reviewed Mar 22, 2016
View reviewed changes
Comment thread lib/matplotlib/mpl-data/stylelib/classic.mplstyle
patch.edgecolor : k
patch.antialiased : True # render patches in antialiased (no jaggies)

hatch.linewidth : 1.0

tacaswell Mar 22, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Did we have 3 (!) different default values for this before?

mdboom Mar 22, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Actually 4, since SVG was "1 point" and Agg was "1 pixel". We could try to emulate that old brokenness if we want (I guess).

tacaswell Mar 23, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

No, I think unify on one is an ok complete break. We should at least document what the old values were. On the off chance we get someone who really cares, they likely only care on one backend (or we would have heard from them sooner!).

mdboom Mar 23, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Agreed. I've documented the old values in the style changes what's new.

mdboom force-pushed the hatch-edge-width branch from b352ddc to 7a982fb Compare March 22, 2016 13:42
jenshnielsen reviewed Mar 22, 2016
View reviewed changes
Comment thread lib/matplotlib/backends/backend_pdf.py Outdated
Op.fill)

self.output(0.1, Op.setlinewidth)
self.output(rcParams['hatch.strokewidth'], Op.setlinewidth)

jenshnielsen Mar 22, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

should this be hatch.linewidth?

mdboom Mar 22, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Indeed. Will fix. Puzzled by why that didn't raise an exception.

QuLogic added API: default changes Changes to default behavior topic: rcparams labels Mar 22, 2016
mdboom force-pushed the hatch-edge-width branch from 3435e36 to f95cccd Compare March 23, 2016 15:32

mdboom commented Mar 23, 2016

Copy link
Copy Markdown
Member Author

Passing Travis. AppVeyor seems blocked for last 3 hours.

QuLogic commented Mar 23, 2016

Copy link
Copy Markdown
Member

I think this is up next, but for some reason, AppVeyor only seems to be building one part of the matrix at a time.

WeatherGod reviewed Mar 24, 2016
View reviewed changes
Comment thread lib/matplotlib/backends/backend_pdf.py
Op.fill)

self.output(0.1, Op.setlinewidth)
self.output(rcParams['hatch.linewidth'], Op.setlinewidth)

WeatherGod Mar 24, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

How significant of a performance hit is it to be constantly re-querying this dictionary?

Also, is it the right thing to access the rcParams this late in the draw process? We are pretty bad at being consistent, but I don't recall doing this sort of rcParam access this late anywhere else. What if someone uses an rc context to set up the hatches, but the savefig call is outside that context?

tacaswell Mar 24, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fair point on being consistent, but there is currently no other API to change this value. The main reason for getting this PR in for 2.0 is to put as rcparam in so we can change it and provide a way to get back to the old version. Adding a proper API for changing this can be done later as a new feature and at that point it should be set at artist creation time, not draw time.

tacaswell commented Mar 27, 2016

Copy link
Copy Markdown
Member

I am happy to merging this as-is to move 2.0 along. This improves things, there is now a way to change the hatch width and it is uniform across all of the backends (that we control) and closes a 3 digit bug!

Still to do are:

  • early binding on all relevant artists (currently all backends will grab the hatch value at draw time instead of artist creation time
  • provide an API to change the hatch linewidth at both creation time and via OO interface
  • have all of the backends pull from the gc instead of rcparams directly.

@mdboom This has one overlap with the test images in the tick centering PR, maybe we should stack them into one merge?

QuLogic commented Mar 27, 2016

Copy link
Copy Markdown
Member

I think the axes spacing in #6129 will also touch many test images and maybe should be folded in as well if you're going to do so already.

tacaswell mentioned this pull request Mar 27, 2016
Closed

tacaswell commented Mar 28, 2016

Copy link
Copy Markdown
Member

I think that #6129 can be done with rcparams in a way that will allow classic mode to not change the tests. This one and #6200 require changes to the tests.

mdboom commented Apr 11, 2016

Copy link
Copy Markdown
Member Author

@tacaswell wrote:

@mdboom This has one overlap with the test images in the tick centering PR, maybe we should stack them into one merge?

Sure -- not sure how best to do that, though.

tacaswell commented Apr 11, 2016

Copy link
Copy Markdown
Member

rebase one of them on the other?

mdboom commented Apr 11, 2016

Copy link
Copy Markdown
Member Author

I think we should just merge this and #6200 separately. It's sort of rebase hell to pull out the test images from this one -- plus it will make bisecting harder later.

tacaswell commented Apr 11, 2016

Copy link
Copy Markdown
Member

fair enough.

tacaswell merged commit 949056c into matplotlib:master Apr 11, 2016
tacaswell added a commit that referenced this pull request Apr 11, 2016
Merge pull request #6198 from mdboom/hatch-edge-width
f367f7b 
ENH/API: Make hatch linewidth an rcParam

This also changes the default hatch width on some backends.

tacaswell commented Apr 11, 2016

Copy link
Copy Markdown
Member

backported to v2.x as f367f7b

This was referenced Nov 5, 2016
Closed
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

API: default changes Changes to default behavior topic: rcparams

Projects

None yet

Milestone

v2.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL