FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Axes.__init__ speedup by QuLogic · Pull Request #8626 · matplotlib/matplotlib · GitHub

Repository navigation

Axes.__init__ speedup - #8626

Merged
tacaswell merged 3 commits into
matplotlib:masterfrom
QuLogic:Axes-init-speedup
Nov 8, 2017
Merged

tacaswell merged 3 commits into
matplotlib:masterfrom
QuLogic:Axes-init-speedup

Conversation

QuLogic commented May 16, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

PR Summary

As noted in #6664, Axes.__init__ is fairly slow due to the eventual call to Axis.reset_ticks. @efiring suggested lazy instantiation and @Kojoley was implementing, but I'm unsure of the status. In the meantime though, we still speed things up by avoiding calling cla too many times.

Using the example from #6664 with v2.0.x:

$ python3 -m timeit \
  -s "from matplotlib.figure import Figure; from matplotlib.axes import Axes; fig = Figure()" \
  "Axes(fig, (0, 0, 1 ,1))"
10 loops, best of 3: 29.7 msec per loop

and master is already a bit faster:

$ python -m timeit \
  -s "from matplotlib.figure import Figure; from matplotlib.axes import Axes; fig = Figure()" \
  "Axes(fig, (0, 0, 1 ,1))"
100 loops, best of 3: 17.7 msec per loop

The trouble with Axes.__init__ is that it does a bit too much clearing; Axes.__init__ calls:

  • Axes._init_axis
    • Creates self.xaxis and self.yaxis
      • Axis.__init__ -> Axis.cla (2 calls per Axes)
    • Registers each Axis with two spines via Spine.register_axis
      • Axis.cla is called on the registered Axis (2 Spine * 2 Axis = 4 calls per Axes)
  • Axes.cla - Necessary to finish initializing the Axes in a clean state, though it's generic for any caller.
    • self.xaxis.cla() / self.yaxis.cla() (2 calls per Axes)
    • self.spines[:].cla() (2 spines per Axis = 4 calls per Axes)

That makes 12 calls to Axis.cla for only 2 Axis objects. Adding some hidden argument to skip these calls as in this PR would reduce those calls by 6, though the speedup is only approximately 4 times:

$ python -m timeit \
  -s "from matplotlib.figure import Figure; from matplotlib.axes import Axes; fig = Figure()" \
  "Axes(fig, (0, 0, 1 ,1))"
100 loops, best of 3: 4.45 msec per loop

PR Checklist

  • [N/A] Has Pytest style unit tests
  • Code is PEP 8 compliant
  • [N/A] New features are documented, with examples if plot related
  • [N/A] Documentation is sphinx and numpydoc compliant
  • [N/A] Added an entry to doc/users/whats_new.rst if major new feature
  • [N/A] Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

QuLogic added this to the 2.1 (next point release) milestone May 16, 2017
QuLogic added 3 commits May 16, 2017 01:30
Both these classes have called their own .cla() method in their
.__init__() method, so don't call it again in Axes.cla() if doing
Axes.__init__().
If the Axis was just created, calling Axis.cla is redundant because it
just happened.
QuLogic force-pushed the Axes-init-speedup branch from 799946c to 4801cbc Compare May 16, 2017 06:18

QuLogic commented May 16, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

It turns out that subclasses (like PolarAxes) tend to override their cla method, so changing their signature is dangerous. Instead, I added an attribute that indicates that the class is still in its __init__ phase.

I'm not sure if the same is true of Spine.register_axis, but seen an error and thus haven't changed it back.

Copy link
Copy Markdown
Member

This seems awkward. Couldn't we take advantage of self.stale instead?

QuLogic commented May 16, 2017

Copy link
Copy Markdown
Member Author

I don't see how. If you mean use it directly, it doesn't hold the same information as what we need. The added flag indicates (essentially) "no settings have been changed from the default", but self.stale means "current settings are different from what was drawn". It has no relationship with default settings for us to use.

If you mean, write something similar to self.stale, that's a lot of work for very little gain and seems rather non-trivial and error-prone to me.

tacaswell modified the milestones: 2.2 (next next feature release), 2.1 (next point release) Aug 29, 2017

efiring commented Nov 8, 2017

Copy link
Copy Markdown
Member

I'm starting to think that the approach to reducing the redundant cla calls here is better than the way I did it in #8752. If so, we should merge this (or a modification of it) and I can change #8752 accordingly. (It needs some more work, but I still want to get that one in, too.)

efiring left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think this looks like a clean and minimally-invasive solution to a long-standing problem.

tacaswell merged commit 1636bed into matplotlib:master Nov 8, 2017
QuLogic deleted the Axes-init-speedup branch November 8, 2017 04:11

dstansby commented Nov 8, 2017

Copy link
Copy Markdown
Member

This appears to have broken lots of stuff on travis! https://travis-ci.org/matplotlib/matplotlib/builds/298909059

I'm going to revert, @QuLogic could you re-open the PR so we can run the changes against the latest revision of master?

Kojoley commented Nov 8, 2017

Copy link
Copy Markdown
Member

This reminds me all kind of strange fails I had faced a year ago at fixing the particular problem.

QuLogic restored the Axes-init-speedup branch November 21, 2017 09:52
efiring mentioned this pull request Dec 4, 2017
6 tasks done
QuLogic modified the milestones: needs sorting, v2.2.0 Feb 13, 2018
QuLogic deleted the Axes-init-speedup branch April 16, 2021 21:54
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants


Back | FazBrowse Home | New Git URL