Correctly compute path extents #16832

brunobeltran · 2020-03-19T11:51:34Z

PR Summary

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

Has Pytest style unit tests
Code is Flake 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

brunobeltran · 2020-03-19T11:59:02Z

I assume this doesn't count as a "major new feature"? If it does I'll add it to next_whats_new/.

lib/matplotlib/bezier.py

anntzer · 2020-03-19T12:19:25Z

Is the first commit elsewhere as a separate PR? If so please refer to it so that we can review things in order.

brunobeltran · 2020-03-19T12:33:12Z

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.

lib/matplotlib/bezier.py

anntzer · 2020-03-21T11:05:49Z

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.

brunobeltran · 2020-03-21T14:10:28Z

@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 :)

brunobeltran

Some comments that might be useful to the reviewer.

lib/matplotlib/path.py

brunobeltran · 2020-03-21T14:19:18Z

lib/matplotlib/path.py

@@ -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):


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.

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?

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.

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.

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.

lib/matplotlib/tests/test_path.py

brunobeltran · 2020-04-09T00:33:27Z

@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!

lib/matplotlib/bezier.py

lib/matplotlib/path.py

brunobeltran · 2020-04-10T02:49:48Z

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!

brunobeltran · 2020-04-10T18:02:57Z

Included @anntzer 's vectorization suggestions (after some hiccups where 3.6 didn't have a no-args lru_cache decorator), sorry @QuLogic that means that the new vectorized __call__ needs to also be reviewed here now.

QuLogic

These comments are not too strong.

doc/users/next_whats_new/2020-03-31-path-size-methods.rst

lib/matplotlib/bezier.py

anntzer · 2020-04-20T12:15:50Z

anyone can merge postci

anntzer reviewed Mar 19, 2020

View reviewed changes

lib/matplotlib/bezier.py Outdated Show resolved Hide resolved

anntzer reviewed Mar 19, 2020

View reviewed changes

lib/matplotlib/bezier.py Outdated Show resolved Hide resolved

brunobeltran force-pushed the path_size_calc branch 4 times, most recently from dd3fec7 to 24b6d39 Compare March 19, 2020 17:57

anntzer reviewed Mar 19, 2020

View reviewed changes

lib/matplotlib/bezier.py Outdated Show resolved Hide resolved

brunobeltran force-pushed the path_size_calc branch 2 times, most recently from b6ca2eb to ca69d46 Compare March 20, 2020 05:00

anntzer added the status: waiting for other PR label Mar 20, 2020

brunobeltran force-pushed the path_size_calc branch from ca69d46 to 1b12e91 Compare March 20, 2020 17:48

brunobeltran mentioned this pull request Mar 20, 2020

Compute area of path #16859

Open

6 tasks

brunobeltran force-pushed the path_size_calc branch from 692c20c to a98a12e Compare March 20, 2020 23:53

brunobeltran force-pushed the path_size_calc branch from a98a12e to 97c748f Compare March 21, 2020 14:06

brunobeltran commented Mar 21, 2020

View reviewed changes

brunobeltran force-pushed the path_size_calc branch 3 times, most recently from 9dc3840 to f0571f0 Compare March 22, 2020 20:46

tacaswell added this to the v3.3.0 milestone Mar 22, 2020

brunobeltran mentioned this pull request Mar 22, 2020

Bezier/Path API Cleanup: fix circular import issue #16812

Merged

6 tasks

brunobeltran force-pushed the path_size_calc branch 2 times, most recently from 225b1f8 to 63ec774 Compare March 23, 2020 19:02

This was referenced Mar 23, 2020

Path length #16888

Open

Path center of mass #16889

Draft

Equal area markers #16891

Draft

QuLogic reviewed Apr 6, 2020

View reviewed changes

lib/matplotlib/tests/test_path.py Outdated Show resolved Hide resolved

brunobeltran force-pushed the path_size_calc branch from ad22228 to b29f403 Compare April 9, 2020 00:32

anntzer reviewed Apr 9, 2020

View reviewed changes

lib/matplotlib/bezier.py Outdated Show resolved Hide resolved

anntzer reviewed Apr 9, 2020

View reviewed changes

lib/matplotlib/path.py Outdated Show resolved Hide resolved

brunobeltran force-pushed the path_size_calc branch 3 times, most recently from 9161982 to b1732b6 Compare April 10, 2020 02:47

brunobeltran mentioned this pull request Apr 10, 2020

DOCS: add examples of how one "should" use Bbox #17090

Merged

6 tasks

brunobeltran force-pushed the path_size_calc branch 3 times, most recently from 5f689df to 1e44dbf Compare April 10, 2020 17:57

QuLogic approved these changes Apr 13, 2020

View reviewed changes

doc/users/next_whats_new/2020-03-31-path-size-methods.rst Outdated Show resolved Hide resolved

lib/matplotlib/bezier.py Show resolved Hide resolved

brunobeltran force-pushed the path_size_calc branch from 1e44dbf to e0d3da1 Compare April 15, 2020 09:10

QuLogic removed the status: needs tests label Apr 15, 2020

brunobeltran mentioned this pull request Apr 17, 2020

Fix clipping of markers in PDF backend. #17163

Merged

2 tasks

brunobeltran force-pushed the path_size_calc branch from e0d3da1 to 5f8493b Compare April 20, 2020 03:05

correctly compute bounding box for path

95a530c

brunobeltran force-pushed the path_size_calc branch from 5f8493b to 95a530c Compare April 20, 2020 11:52

brunobeltran mentioned this pull request Apr 20, 2020

Stroked path width #17198

Draft

6 tasks

anntzer approved these changes Apr 20, 2020

View reviewed changes

anntzer merged commit e841b51 into matplotlib:master Apr 20, 2020

dopplershift mentioned this pull request Apr 21, 2020

margins does not handle bézier curves #7184

Closed

dstansby mentioned this pull request Jul 9, 2020

plt.bar with nan input fails rendering in notebook using 3.3.0rc1 #17868

Closed

jklymak mentioned this pull request Jul 21, 2020

Major speed regression introduced in "plt.bar" definition clipping between 3.0.3 and 3.3.0. #17974

Closed

anntzer mentioned this pull request Jan 1, 2021

Improve autoscaling for high order Bezier curves #19214

Merged

dstansby mentioned this pull request Jan 2, 2021

Ignore non-draw codes when calculating path extent #19216

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly compute path extents #16832

Correctly compute path extents #16832

brunobeltran commented Mar 19, 2020 •

edited

Loading

brunobeltran commented Mar 19, 2020

anntzer commented Mar 19, 2020

brunobeltran commented Mar 19, 2020

anntzer commented Mar 21, 2020

brunobeltran commented Mar 21, 2020

brunobeltran left a comment

brunobeltran Mar 21, 2020

anntzer Mar 22, 2020

brunobeltran Mar 22, 2020 •

edited

Loading

brunobeltran Mar 22, 2020 •

edited

Loading

brunobeltran Mar 22, 2020

brunobeltran commented Apr 9, 2020 •

edited

Loading

brunobeltran commented Apr 10, 2020

brunobeltran commented Apr 10, 2020

QuLogic left a comment

anntzer commented Apr 20, 2020

Correctly compute path extents #16832

Correctly compute path extents #16832

Conversation

brunobeltran commented Mar 19, 2020 • edited Loading

PR Summary

PR Checklist

brunobeltran commented Mar 19, 2020

anntzer commented Mar 19, 2020

brunobeltran commented Mar 19, 2020

anntzer commented Mar 21, 2020

brunobeltran commented Mar 21, 2020

brunobeltran left a comment

Choose a reason for hiding this comment

brunobeltran Mar 21, 2020

Choose a reason for hiding this comment

anntzer Mar 22, 2020

Choose a reason for hiding this comment

brunobeltran Mar 22, 2020 • edited Loading

Choose a reason for hiding this comment

brunobeltran Mar 22, 2020 • edited Loading

Choose a reason for hiding this comment

brunobeltran Mar 22, 2020

Choose a reason for hiding this comment

brunobeltran commented Apr 9, 2020 • edited Loading

brunobeltran commented Apr 10, 2020

brunobeltran commented Apr 10, 2020

QuLogic left a comment

Choose a reason for hiding this comment

anntzer commented Apr 20, 2020

brunobeltran commented Mar 19, 2020 •

edited

Loading

brunobeltran Mar 22, 2020 •

edited

Loading

brunobeltran Mar 22, 2020 •

edited

Loading

brunobeltran commented Apr 9, 2020 •

edited

Loading