Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
I haven't looked in depth into this, but why did#16832 (which fixed#7184) not fix this? attn@brunobeltran perhaps? |
It looks like#16832 fixed |
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. |
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. |
😆 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. |
a105bba
to0482fff
CompareThis is now good to go, and was a pretty small change (in terms of lines of code) in the end. |
ping for a second review on this! |
There was a problem hiding this 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?
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
if len(vertices): | ||
vertices = np.row_stack(vertices) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@dstansby popping to draft until you can address review comments, but feel free to pop back on the queue! |
I think this is because the (0, 0) point is sticky, and is now being snapped into view. |
I don't understand how that happens? |
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. |
It seems that autoscaling doesn't ever take into account the labels added by As such, I'd advocate for merging this to solve the Bezier scaling issue, and track the issues with |
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 commentedDec 28, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
👍 - I updated all the test images that are changing anyway to |
To check my understanding the pie images are regenerated because:
If my understanding is correct@dstansby (or anyone else) can (self-)merge. |
Uh oh!
There was an error while loading.Please reload this page.
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
Before:


After: