Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Improve autoscaling for high order Bezier curves#19214

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
jklymak merged 2 commits intomatplotlib:mainfromdstansby:bezier-autoscale
Feb 1, 2022

Conversation

dstansby
Copy link
Member

@dstansbydstansby commentedJan 1, 2021
edited
Loading

Overview

Currently every control point is used when autoscaling with a Bezier curve, but for high order curves this isn't desired, as control points that aren't the start or end point do not lie on the actual curve. A better way to calculate points for autoscaling is to include the start point, end point, and any extrema on the curve.

This PR introduces a new method to convert a path from a set of Bezier segments to straight lines connecting the start, extrema, and end points. The bounding box of these points is then the bounding box of the overall Path, so it can be used for autoscaling.

This leverages the new functionality in#16832 introduced to properly calculate the bounding box of a complex Bezier curve, and makes sure it is used in the autoscaling.

Fixes#19174

Example

frommatplotlib.pathimportPathfrommatplotlib.patchesimportPathPatchimportmatplotlib.pyplotaspltverts= [[-1,0],         [0,-1],         [1,0],         [1,0]]codes= [Path.MOVETO,Path.CURVE3,Path.CURVE3,Path.CLOSEPOLY]p=Path(verts,codes)fig,ax=plt.subplots()ax.add_patch(PathPatch(p))ax.autoscale()plt.show()

Before:
before
After:
after

@anntzer
Copy link
Contributor

I haven't looked in depth into this, but why did#16832 (which fixed#7184) not fix this? attn@brunobeltran perhaps?

@dstansby
Copy link
MemberAuthor

It looks like#16832 fixedPath.get_extents, butPath.get_extents doesn't seem to be used inAxes._update_patch_limits, which controls the autoscaling, and which I've tried to patch in this PR. Thanks for pointing out#16832, I can probably replace my horrendously large new function with something smaller from that.

@QuLogic
Copy link
Member

So you're trying to replace individual points by the extent corners only? How does this affect log scales? Because the fix in#18642 was to not do that as much.

@dstansby
Copy link
MemberAuthor

Log scaling seems fine; the failing examples seem to be mainly pie charts. Most of the changes look reasonable, but there's a couple that definitely were better before this patch, so I need to do a bit more investigating.

@dstansby
Copy link
MemberAuthor

😆 it turns out that log scaling wasn't quite fine. I think I've fixed it this time, but will see how the tests go.

@dstansbydstansbyforce-pushed thebezier-autoscale branch 2 times, most recently froma105bba to0482fffCompareOctober 8, 2021 18:01
@dstansbydstansby marked this pull request as ready for reviewOctober 8, 2021 18:42
@dstansby
Copy link
MemberAuthor

This is now good to go, and was a pretty small change (in terms of lines of code) in the end.

@jklymak
Copy link
Member

ping for a second review on this!

Copy link
Member

@QuLogicQuLogic left a comment

Choose a reason for hiding this comment

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

The change inlib/matplotlib/tests/baseline_images/test_axes/pie_frame_grid.png does not seem better?

Comment on lines +2385 to +2387
if len(vertices):
vertices = np.row_stack(vertices)
Copy link
Member

Choose a reason for hiding this comment

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

Do you not want to return here if empty any more?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Perhaps? It's a private helper function and doesn't seem to break anything though - I guess we're always passing in input that has some vertices.

Copy link
Member

Choose a reason for hiding this comment

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

That's becausetransform andupdate_datalim are safe to empty lists. I think this is more of an optimization to avoid decomposing the transform, etc.

@jklymak
Copy link
Member

@dstansby popping to draft until you can address review comments, but feel free to pop back on the queue!

@jklymakjklymak marked this pull request as draftNovember 29, 2021 08:14
@dstansby
Copy link
MemberAuthor

The change inlib/matplotlib/tests/baseline_images/test_axes/pie_frame_grid.png does not seem better?

I think this is because the (0, 0) point is sticky, and is now being snapped into view.

@QuLogic
Copy link
Member

I don't understand how that happens?

@dstansby
Copy link
MemberAuthor

My suspicion is that this PR has uncovered another bug somewhere in (auto)scaling - I'll try and work out what's going on and report back.

@dstansby
Copy link
MemberAuthor

I'm going to leave a trail of crumbs as I investigate the funny axes scaling.

The issue only seems to appear withplt.style.use('classic') (which the tests use by default), where thepie text doesn't seem to be taken into account in the scaling:
pie

importmatplotlib.pyplotaspltplt.style.use('classic')# The slices will be ordered and plotted counter-clockwise.labels='Frogs','Hogs','Dogs','Logs'sizes= [15,30,45,10]colors= ['yellowgreen','gold','lightskyblue','lightcoral']# only "explode" the 2nd slice (i.e. 'Hogs')explode= (0,0.1,0,0)plt.pie(sizes,explode=explode,labels=labels,colors=colors,autopct='%1.1f%%',startangle=90,wedgeprops={'linewidth':0},frame=True,center=(2,2))# Set aspect ratio to be equal so that pie is drawn as a circle.plt.axis('equal')plt.show()

@dstansbydstansby marked this pull request as ready for reviewDecember 28, 2021 16:08
@dstansbydstansby added this to thev3.6.0 milestoneDec 28, 2021
@dstansby
Copy link
MemberAuthor

It seems that autoscaling doesn't ever take into account the labels added bypie() (I've opened#22057 to track this). By coincidence the older test images never encountered this issue, but one of the new test images does because of this fix.

As such, I'd advocate for merging this to solve the Bezier scaling issue, and track the issues withpie() autoscaling separately.

@jklymak
Copy link
Member

That sounds reasonable. Can you fix the test so that isnot classic and we can track pie issues separately. Definitely ping if you need a review.

@dstansby
Copy link
MemberAuthor

dstansby commentedDec 28, 2021
edited
Loading

Can you fix the test so that isnot classic and we can track pie issues separately.

👍 - I updated all the test images that are changing anyway tompl20 style.

@tacaswell
Copy link
Member

To check my understanding the pie images are regenerated because:

  • previously the auto scaling was wrong, but worked out because the limits were too big because the control points were significantly away from the
  • with this patch the scaling was too tight as under the "classic" style was being right at the edges of the pie chart which makes the text go out of the axes
  • but with v2 setting there is a margin so there is enough space for the text again

If my understanding is correct@dstansby (or anyone else) can (self-)merge.

jklymak reacted with thumbs up emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@QuLogicQuLogicQuLogic left review comments

@tacaswelltacaswelltacaswell approved these changes

@jklymakjklymakjklymak approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.6.0
Development

Successfully merging this pull request may close these issues.

connectionstyle arc3 with high rad value pushes up data interval of x-axis and y-axis.
5 participants
@dstansby@anntzer@QuLogic@jklymak@tacaswell

[8]ページ先頭

©2009-2025 Movatter.jp