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

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

Merged
anntzer merged 1 commit intomatplotlib:masterfrombrunobeltran:path_size_calc
Apr 20, 2020

Conversation

brunobeltran
Copy link
Contributor

@brunobeltranbrunobeltran commentedMar 19, 2020
edited
Loading

PR Summary

Closes#16830 and#7184.

This code will be needed to implementMarkerStyle 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 isFlake 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
Copy link
ContributorAuthor

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

@anntzer
Copy link
Contributor

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

@brunobeltran
Copy link
ContributorAuthor

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.

@brunobeltranbrunobeltranforce-pushed thepath_size_calc branch 4 times, most recently fromdd3fec7 to24b6d39CompareMarch 19, 2020 17:57
@anntzer
Copy link
Contributor

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
Copy link
ContributorAuthor

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

Copy link
ContributorAuthor

@brunobeltranbrunobeltran left a 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.

@@ -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):
Copy link
ContributorAuthor

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.

Copy link
Contributor

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?

Copy link
ContributorAuthor

@brunobeltranbrunobeltranMar 22, 2020
edited
Loading

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.

Copy link
ContributorAuthor

@brunobeltranbrunobeltranMar 22, 2020
edited
Loading

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.

Copy link
ContributorAuthor

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.

@brunobeltranbrunobeltranforce-pushed thepath_size_calc branch 3 times, most recently from9dc3840 tof0571f0CompareMarch 22, 2020 20:46
@tacaswelltacaswell added this to thev3.3.0 milestoneMar 22, 2020
@brunobeltranbrunobeltranforce-pushed thepath_size_calc branch 2 times, most recently from225b1f8 to63ec774CompareMarch 23, 2020 19:02
This was referencedMar 23, 2020
@brunobeltran
Copy link
ContributorAuthor

brunobeltran commentedApr 9, 2020
edited
Loading

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

@brunobeltranbrunobeltranforce-pushed thepath_size_calc branch 3 times, most recently from9161982 tob1732b6CompareApril 10, 2020 02:47
@brunobeltran
Copy link
ContributorAuthor

Wow, extremely surprising defaults forBbox.update_from_data_xy caused a regression (needed to setignore=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!

@brunobeltranbrunobeltranforce-pushed thepath_size_calc branch 3 times, most recently from5f689df to1e44dbfCompareApril 10, 2020 17:57
@brunobeltran
Copy link
ContributorAuthor

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

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.

These comments are not too strong.

@anntzer
Copy link
Contributor

anyone can merge postci

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

@jklymakjklymakjklymak left review comments

@QuLogicQuLogicQuLogic approved these changes

@anntzeranntzeranntzer approved these changes

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

Successfully merging this pull request may close these issues.

_path.get_extents does not correctly handle bezier curves
5 participants
@brunobeltran@anntzer@jklymak@QuLogic@tacaswell

[8]ページ先頭

©2009-2025 Movatter.jp