Skip to content

Close clipped paths #4186

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 4, 2015
Merged

Close clipped paths #4186

merged 3 commits into from
Mar 4, 2015

Conversation

mdboom
Copy link
Member

@mdboom mdboom commented Mar 4, 2015

Fix #4185.

@WeatherGod
Copy link
Member

one of the failures has to do with the recent fix to trifinder. Did numpy
1.6 not have "order" kwarg for its copy function? Also, didn't that PR pass
all of the tests?

On Tue, Mar 3, 2015 at 7:00 PM, Michael Droettboom <notifications@github.com

wrote:

Fix #4185 #4185.

You can view, comment on, or merge this pull request online at:

#4186
Commit Summary

  • Close clipped paths

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#4186.

extend='both',)
plt.colorbar(im,cax=ax4,cmap=cmap,orientation='horizontal',
extend='both',extendrect=True)
plt.colorbar(im,cax=ax5,cmap=cmap,orientation='horizontal',
Copy link
Member

Choose a reason for hiding this comment

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

this test is failing PEP8 because of the lack of spaces around the commas.

@tacaswell
Copy link
Member

Would it be worth making the line width on the tests very large (given that we don't have exact tests).

ax4 = fig.add_axes([0.05, 0.25, 0.9, 0.1])
ax5 = fig.add_axes([0.05, 0.05, 0.9, 0.1])

cmap = cm.jet
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the image files would be smaller if the map had fewer colors?

Copy link
Member

Choose a reason for hiding this comment

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

Also in the spirit of doing-in-jet should we use a different color map ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

How about changing the cmap to this?:

...
from matplotlib.colors import ListedColormap
cmap = ListedColormap([(.25,.25,.25,1.0),(.5,.5,.5,1.0),(.75,.75,.75,1.0)])
cmap.set_under('w')
cmap.set_over('w')

im = ax1.pcolormesh(np.linspace(0,10,16).reshape((4,4)),cmap=cmap)
...

@mdboom mdboom force-pushed the close-clipped-paths branch from df8eebd to 5b82af6 Compare March 4, 2015 16:42
@mdboom
Copy link
Member Author

mdboom commented Mar 4, 2015

I think everything above is now addressed.

@tacaswell
Copy link
Member

The clipping is different for the first on the pdf result and the svg/png result.
so

@mdboom
Copy link
Member Author

mdboom commented Mar 4, 2015

@tacaswell: Yes, the default join style appears to be different (miter vs. bevel). I'd rather treat that as a separate issue, as that's likely to affect many more test cases.

@u55
Copy link
Contributor

u55 commented Mar 4, 2015

@tacaswell @mdboom

The clipping is different for the first on the pdf result and the svg/png result.

The clipping of the svg image is dependent on the viewer. On my windows machine, they look different in Chrome versus Firefox.

Chrome:
chrome_svg
Firefox:
firefox_svg

@u55
Copy link
Contributor

u55 commented Mar 4, 2015

Edit:
Also, in inkscape, the svg image looks the same as in Firefox. So Chrome is the oddball.

I'm sorry, I said that backwards. Inkscape is the same as Chrome. Firefox is the oddball.

@mdboom
Copy link
Member Author

mdboom commented Mar 4, 2015

The difference we are seeing is that the stroke width and angle are high enough that the miter limit is going into effect. Here's a good explanation of miter limit:

http://tutorials.jenkov.com/svg/stroke.html#stroke-miterlimit

The default miter limit is somewhat dependent on the renderer (it's specified in the SVG standard, for example, but I'm not certain every implementation follows it), and what we should be doing is setting it explicitly to get uniform behavior.

Ideally, we'd add an API for the user to set the miter limit explicitly, but we've never had that before (i.e. it's not a regression to not have it), and doing so is sort of beyond the scope of this PR. Instead, I think for now we just set it to some reasonable maximum (effectively disabling it), since if the user wants the miters to be limited, they can always set the join style to bevel anyway (not exactly the same thing, of course, but good enough for now).

Now that I have a better understanding of what's going on here, I'll probably just add the fix to this PR (since the test here serves as a good test for the other). Stay tuned...

@mdboom mdboom force-pushed the close-clipped-paths branch from 1d97440 to a93f4b9 Compare March 4, 2015 19:55
@mdboom
Copy link
Member Author

mdboom commented Mar 4, 2015

Ok. The miter limit change is now included. It did not affect any existing tests, other than the new one in this PR.

@mdboom mdboom force-pushed the close-clipped-paths branch from a93f4b9 to 3b63c19 Compare March 4, 2015 22:41
@mdboom
Copy link
Member Author

mdboom commented Mar 4, 2015

I've updated the test images so the miter points don't extend beyond the edge of the image.

efiring added a commit that referenced this pull request Mar 4, 2015
@efiring efiring merged commit be305a3 into matplotlib:master Mar 4, 2015
@efiring
Copy link
Member

efiring commented Mar 4, 2015

@mdboom, thank you for taking care of this so quickly. It looks like it needs to be cherry-picked to the color_overhaul branch, correct? I would be more comfortable with you or @tacaswell doing that; I'm shaky with that git operation.

@u55
Copy link
Contributor

u55 commented Mar 5, 2015

@mdboom , thank you. I installed master on my Linux machine and your fix works great. Again thanks for the fast work.

@tacaswell
Copy link
Member

@mdboom This touches code that was greatly changed as part of the cxx refactoring. Can you have a look at back-porting this?

@mdboom mdboom deleted the close-clipped-paths branch November 9, 2015 02:35
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.

Colorbar outline has broken path in vector backends
5 participants