| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
I assume this doesn't count as a "major new feature"? If it does I'll add it to next_whats_new/. |
Sorry, something went wrong.
|
Is the first commit elsewhere as a separate PR? If so please refer to it so that we can review things in order. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
Specifically 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. |
Sorry, something went wrong.
|
@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 :) |
Sorry, something went wrong.
There was a problem hiding this comment.
Some comments that might be useful to the reviewer.
Sorry, something went wrong.
| curr_vertices = np.append(curr_vertices, next(vertices)) | ||
| yield curr_vertices, code | ||
|
|
||
| def iter_curves(self, **kwargs): |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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?
Sorry, something went wrong.
There was a problem hiding this comment.
I'm happy to make this a private helper method that is called by iter_segments when the parameter include_starts is passed, but I worry about bloating iter_segments with a bunch of options.
Especially because it's really obnoxious that .iter_segments() returns a flattened 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 of reshape calls), I'd have to add another parameter as well flattened=False which determines whether to flatten the (naturally (N,2)-shaped) array of control points before returning it.
Sorry, something went wrong.
There was a problem hiding this comment.
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 and flatten_vertices to iter_segments instead of just making a separate iter_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.
Sorry, something went wrong.
There was a problem hiding this comment.
Pushed up a new version where the generator yields BezierSegments and is named iter_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 to iter_segments.
Sorry, something went wrong.
|
@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! |
Sorry, something went wrong.
|
Wow, extremely surprising defaults for Bbox.update_from_data_xy caused a regression (needed to set ignore=False explicitly, otherwise bug would only appear during testing and not on command line :/) But new, fully vectorized (and easier to read) version of the path extents code now pushed up, ready for re-review! |
Sorry, something went wrong.
There was a problem hiding this comment.
These comments are not too strong.
Sorry, something went wrong.
|
anyone can merge postci |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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