Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Correctly compute path extents#16832
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 assume this doesn't count as a "major new feature"? If it does I'll add it to |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Is the first commit elsewhere as a separate PR? If so please refer to it so that we can review things in order. |
Ooops, you're right. I got mixed up and thought it was the other way around. This depends on#16812, which in turn depends on your#14199 being merged, which I just realized hasn't happened yet. My bad. |
dd3fec7
to24b6d39
CompareUh oh!
There was an error while loading.Please reload this page.
b6ca2eb
toca69d46
CompareSpecifically for "stacked" PRs like this I would suggest squashing them to a single or a small number of commits cleanly on the top of the parent PR (or master, if the parent PR has been merged) to simplify the review. Otherwise it's too hard for the reviewer (well, at least for me) to keep track of what's done where. |
@anntzer Thanks for the tip! I squashed all the PRs in this "stack" into one commit each, so you can review the relevant parts easily :) |
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.
Some comments that might be useful to the reviewer.
Uh oh!
There was an error while loading.Please reload this page.
lib/matplotlib/path.py Outdated
@@ -421,6 +432,54 @@ def iter_segments(self, transform=None, remove_nans=True, clip=None, | |||
curr_vertices = np.append(curr_vertices, next(vertices)) | |||
yield curr_vertices, code | |||
def iter_curves(self, **kwargs): |
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.
I did try to make iter_segments work first. But I found that here (and#16859, and upcoming PR on center of mass), I kept repeating this logic across each function I wrote so that I could get all the control points of each curve, so I went ahead and added this generator.
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.
A problem is that naively you'd thing the difference between iter_segments and iter_curves is that one returns segments and the other returns curves, but actually it's something completely different: one excludes the start point of each segment/curve and the other includes it. So should this instead be a kwarg to iter_segments, e.g.include_starts
?
brunobeltranMar 22, 2020 • 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.
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.
I'm happy to make this a private helper method that is called byiter_segments
when the parameterinclude_starts
is passed, but I worry about bloatingiter_segments
with a bunch of options.
Especially because it'sreally obnoxious that.iter_segments()
returns aflattened sequence of coordinate pairs, so in order to create the desired API (more importantly to not doom anyone using this code in the future to a lifetime ofreshape
calls), I'd have to add another parameter as wellflattened=False
which determines whether to flatten the (naturally (N,2)-shaped) array of control points before returning it.
brunobeltranMar 22, 2020 • 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.
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.
Oh, and currently,.iter_segments()
returns a vertex that we're supposed to be ignoring (CLOSEPOLY) instead of correctly returning the first vertex of that stroke.
This is almost certainly a bug, but would probably want its own PR.
Adding the CLOSEPOLY bugfix,include_starts
andflatten_vertices
toiter_segments
instead of just making a separateiter_curves
makes the code significantly noisier, but if that's not as big of a priority as minimizing new API surface, I'm happy to do that.
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.
Pushed up a new version where the generator yields BezierSegments and is namediter_bezier
. I think this solves the problem that you (correctly) pointed out of the name.iter_curves
not being intuitive, along with preventing me from having to make multiple, significant modifications toiter_segments
.
9dc3840
tof0571f0
Compare225b1f8
to63ec774
CompareUh oh!
There was an error while loading.Please reload this page.
brunobeltran commentedApr 9, 2020 • 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.
@QuLogic, Fixed tests to follow the "parametrize" conventions I found in tests/test_axes.py. Let me know if there are still changes I could make to have the tests be more in line with the mpl conventions! |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
9161982
tob1732b6
CompareWow, extremely surprising defaults for But new, fully vectorized (and easier to read) version of the path extents code now pushed up, ready for re-review! |
5f689df
to1e44dbf
CompareThere 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.
These comments are not too strong.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
anyone can merge postci |
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
Closes#16830 and#7184.
This code will be needed to implement
MarkerStyle
normalization correctly (building on the work now approved in#16773).Roadmap:
#16812 (*) <- (This PR) (*) <-#16859 (*) <-#16888 <-#16889 (*) <-#16891 (MarkerStyle improvements!)
"<-" means "depends on", and "(*)" marks PRs whose implementations are complete and fully ready for review.
PR Checklist