8000 FIX: fastpath clipped artists by jklymak · Pull Request #14445 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

FIX: fastpath clipped artists #14445

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
Jun 8, 2019
Merged

FIX: fastpath clipped artists #14445

merged 1 commit into from
Jun 8, 2019

Conversation

jklymak
Copy link
Member
@jklymak jklymak commented Jun 4, 2019 •
edited
Loading

PR Summary

In 3.x ax.get_tightbbox() started to include all artists on the axes when it was called. This is an expected performance hit. @tacaswell suggested that perhaps if the artist is clipped by the axes patch this is not necessary.

The change here implements that solution - if the artist is clipped and the clip path is contained in the axes clip path, then artist.get_tightbbox() is no longer called for that artist.

Example:

import matplotlib.pyplot as plt
import time
import numpy as np

fig, ax = plt.subplots()

ax.plot(np.arange(2e7) + 1, np.arange(2e7))
ax.set_xlim([1, 40])

st = time.time()
for i in range(40):
    fig.tight_layout()
    plt.draw()
print(time.time() - st)
plt.show()

Old:

18.68

New:

0.4187

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

anntzer
anntzer reviewed Jun 4, 2019
View reviewed changes
lib/matplotlib/artist.py Outdated
@@ -249,6 +249,23 @@ def get_window_extent(self, renderer):
"""
return Bbox([[0, 0], [0, 0]])

def get_clip_bbox_extents(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps call this get_clip_extents? (otherwise it's not clear whether it's just the extents of the clip box or of the intersection of the clip_box and the clip_path)

Copy link
Member Author

Choose a reason for hiding this comment

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

How about get_clip_bbox. I guess this should also be private, just so we don't have to worry about changing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for private as usual ;)
get_clip_bbox sounds too close to get_clip_box to me...

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, private and _get_clipping_extent_bbox was what I decided on. It is an extent (rather than the actual clip path) and it returns a Bbox.

@jklymak jklymak force-pushed the fix-fastpath-clipped-artists branch 2 times, most recently from f6dbc7f to c4c7473 Compare June 4, 2019 18:36
@jklymak jklymak marked this pull request as ready for review June 4, 2019 19:36
@jklymak jklymak added the topic: geometry manager LayoutEngine, Constrained layout, Tight layout label Jun 4, 2019
@jklymak jklymak added this to the v3.1.1 milestone Jun 4, 2019
@jklymak
Copy link
Member Author
8000 jklymak commented Jun 4, 2019

Trying to sneak in to 3.1.1 since this can be a pretty big speedup for some cases. But not Release Critical if it doesn't get in...

anntzer
anntzer reviewed Jun 4, 2019
View reviewed changes
lib/matplotlib/artist.py Outdated
@@ -249,6 +249,23 @@ def get_window_extent(self, renderer):
"""
return Bbox([[0, 0], [0, 0]])

def _get_clipping_extent_bbox(self):
"""
Return the intersection of the clip_path and clip_box for this
Copy link
Contributor

Choose a reason for hiding this comment

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

"Return the extents of the intersection..."

anntzer
anntzer reviewed Jun 4, 2019
View reviewed changes
lib/matplotlib/artist.py Outdated
def _get_clipping_extent_bbox(self):
"""
Return the intersection of the clip_path and clip_box for this
artist. Returns None if both of these are None, or ``get_clip_on``
Copy link
Contributor
@anntzer anntzer Jun 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

", or None if either..."

anntzer
anntzer reviewed Jun 4, 2019
View reviewed changes
lib/matplotlib/axes/_base.py Outdated
if clip_extent is not None:
clip_extent = mtransforms.Bbox.intersection(clip_extent,
axbbox)
inside_axes = np.all(clip_extent.extents == axbbox.extents)
Copy link
Contributor

Choose a reason for hiding this comment

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

if not inside_axes: continue

and then below you don't need to re-check for Noneness of clip_extent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Refactored to make this cleaner...

Copy link
Member Author

Choose a reason for hiding this comment

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

... though I have a mild aversion to continue. Somewhere I got accustomed to continue being bad style...

anntzer
anntzer approved these changes Jun 4, 2019
View reviewed changes
Copy link
Contributor
@anntzer anntzer left a comment

Choose a reason for hiding this comment

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

Only minor style points, but overall looks nice.

@jklymak jklymak force-pushed the fix-fastpath-clipped-artists branch from c4c7473 to 1f889a8 Compare June 4, 2019 20:42
@jklymak jklymak force-pushed the fix-fastpath-clipped-artists branch from 1f889a8 to 2eb6772 Compare June 4, 2019 20:44
@tacaswell tacaswell merged commit 8555717 into matplotlib:master Jun 8, 2019
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Jun 8, 2019
@tacaswell
Backport PR matplotlib#14445: FIX: fastpath clipped artists
be393c5
@meeseeksmachine meeseeksmachine mentioned this pull request Jun 8, 2019
Merged
@tacaswell
Copy link
Member

Thanks @jklymak !

timhoffm added a commit that referenced this pull request Jun 9, 2019
@timhoffm
Merge pull request #14504 from meeseeksmachine/auto-backport-of-pr-14…
1c62bb7 
…445-on-v3.1.x

Backport PR #14445 on branch v3.1.x (FIX: fastpath clipped artists)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anntzer anntzer anntzer approved these changes

Assignees
No one assigned
Labels
topic: geometry manager LayoutEngine, Constrained layout, Tight layout
Projects
None yet
Milestone
v3.1.1
Development

Successfully merging this pull request may close these issues.
3 participants
@jklymak @tacaswell @anntzer
0