Skip to content

FIX: fastpath clipped artists#14445

Merged
tacaswell merged 1 commit into
matplotlib:masterfrom
jklymak:fix-fastpath-clipped-artists
Jun 8, 2019
Merged

FIX: fastpath clipped artists#14445
tacaswell merged 1 commit into
matplotlib:masterfrom
jklymak:fix-fastpath-clipped-artists

Conversation

@jklymak
Copy link
Copy Markdown
Member

@jklymak jklymak commented Jun 4, 2019

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

Comment thread lib/matplotlib/artist.py Outdated
"""
return Bbox([[0, 0], [0, 0]])

def get_clip_bbox_extents(self):
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

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...

Comment thread lib/matplotlib/artist.py Outdated

def _get_clipping_extent_bbox(self):
"""
Return the intersection of the clip_path and clip_box for this
Copy link
Copy Markdown
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..."

Comment thread 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
Copy Markdown
Contributor

@anntzer anntzer Jun 4, 2019

Choose a reason for hiding this comment

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

", or None if either..."

Comment thread 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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...

Copy link
Copy Markdown
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
Copy link
Copy Markdown
Member

Thanks @jklymak !

timhoffm added a commit that referenced this pull request Jun 9, 2019
…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

Labels

topic: geometry manager LayoutEngine, Constrained layout, Tight layout

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants