8000 Remove list(zip(...)) when unnecessary. by anntzer · Pull Request #10102 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Remove list(zip(...)) when unnecessary. #10102

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Dec 27, 2017
Merged

Remove list(zip(...)) when unnecessary. #10102

merged 1 commit into from
Dec 27, 2017

Conversation

anntzer
Copy link
Contributor
@anntzer anntzer commented Dec 26, 2017

Replace by zip(...) when the result is immediately unpacked. Replace
by np.column_stack when operating on arrays or when the result will be
immediately converted to an array (e.g. by the Path constructor), as
column_stack is faster in these cases.

(This commit does not remove all instances of list(zip(...)).)

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 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

@anntzer anntzer force-pushed the remove-list-zip branch 3 times, most recently from a1b081e to 0754b29 Compare December 26, 2017 23:30
@tacaswell tacaswell added this to the v2.2 milestone Dec 27, 2017
efiring
efiring approved these changes Dec 27, 2017
View reviewed changes
Copy link
Member
@efiring efiring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor quibble, not a blocker.

lib/matplotlib/bezier.py Outdated
@@ -279,7 +278,7 @@ def split_path_inout(path, inside, tolerence=0.01, reorder_inout=False):
codes_left = [Path.CURVE4, Path.CURVE4, Path.CURVE4]
codes_right = [Path.MOVETO, Path.CURVE4, Path.CURVE4, Path.CURVE4]
else:
raise ValueError()
raise ValueError
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of returning the class, it would be better to supply some sort of error message, wouldn't it? Leaving the empty parenthesis is at least a flag for someone to supply that message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edited accordingly. Given the context of the code I believe this line should never be reached (it is effectively following a switch on the result of an internal routine).

QuLogic
QuLogic reviewed Dec 27, 2017
View reviewed changes
examples/api/collections.py Outdated
@@ -30,16 +30,14 @@
theta = np.linspace(0, 2*np.pi, nverts)
xx = r * np.sin(theta)
yy = r * np.cos(theta)
spiral = list(zip(xx, yy))
spiral = np.column_stack([xx, yy])

# Make some offsets
# Fixing random state for reproducibility
rs = np.random.RandomState(19680801)


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might as well remove this extra line while you're here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

examples/api/collections.py Outdated
@@ -30,16 +30,14 @@
theta = np.linspace(0, 2*np.pi, nverts)
xx = r * np.sin(theta)
yy = r * np.cos(theta)
spiral = list(zip(xx, yy))
spiral = np.column_stack([xx, yy])

# Make some offsets
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And move this comment down to above xyo where it should be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

lib/matplotlib/bezier.py Outdated
left, right = split_bezier_intersecting_with_closedpath(bp,
inside,
tolerence)
bp = np.column_stack([bezier_path[::2], bezier_path[1::2]])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

np.reshape(bezier_path, (-1, 2)), no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed

@anntzer
Remove list 8000 (zip(...)) when unnecessary.
81b0efc 
Replace by zip(...) when the result is immediately unpacked.  Replace
by np.column_stack when operating on arrays or when the result will be
immediately converted to an array (e.g. by the Path constructor), as
column_stack is faster in these cases.

(This commit does not remove *all* instances of list(zip(...)).)
@anntzer
Copy link
Contributor Author
anntzer commented Dec 27, 2017

added some more simplifications to bezier code as well.

@QuLogic QuLogic merged commit af970dd into matplotlib:master Dec 27, 2017
@anntzer anntzer deleted the remove-list-zip branch December 27, 2017 18:04
@QuLogic QuLogic modified the milestones: needs sorting, v2.2.0 Feb 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic left review comments

@efiring efiring efiring approved these changes

@tacaswell tacaswell tacaswell approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.2.0
Development

Successfully merging this pull request may close these issues.
4 participants
@anntzer @efiring @tacaswell @QuLogic
0