-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Lazy initialization of axis default tick list #9727
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Lazy initialization of axis default tick list #9727
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be targetting master
; it's not a bug fix.
Also, it appears you may have forgotten to set your name in your git config.
lib/matplotlib/axis.py
Outdated
self._lastNumMajorTicks = 1 | ||
self._lastNumMinorTicks = 1 | ||
if not isinstance(self.majorTicks, LazyDefaultTickList): | ||
del self.majorTicks[:] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't need this any more since the list is being removed, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guess you're right. I was wondering anyway, why the previous code bothered to del every item from the list and not just created a new list (self.majorTicks=[]
would have been my expectation in the original code).
Also, can you tell if there's still a need for self._lastNumMajorTicks
? I haven't looked into this so far. It feels like a helper state, that might not be necessary. If you don't know right now, I can check myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it worked this way so that any use of self.majorTicks
would point to the same list. It's interesting that all the tests still pass though, as clearly this new class does not make that same guarantee.
I'm not clear on why this code needs to be wrapped in a class. I'm sure its just my ignorance... |
You mean why not start with an empty list? Because many things subtly break and it's a giant rabbit hole... |
Could we at least add and |
@jklymak We want to have a list with always at least one element. But we don't want to actually create that element, unless we really use it. The creation of unused ticks is a significant overhead. Of course, you can implement that logic without a class, but that would mean, that you'll have conditional logic everytime you use the list, i.e. more complex code and also the checks are still run later when the list is there for sure (probably not relevant performance-wise though). The class allows to get both, the simple API of a plain list and the lazy initialization. We could add |
Thank you all for your feedback so far. This code is intended as a working prototype to get the discussion started. There might still be room for some improvement. I will look into this further on the weekend. |
If I understand it correctly, I wonder if the following approach could be made to work instead: turn |
Oh, I get it. Sorry for being slow. OTOH, does this really speed anything up if you actually access the axes? I don't understand how this helps real-world cases, but again, I'm sure I'm just being slow. EDIT: OK: I checked, and it indeed speeds things up. I'm still not at all clear why. If I do x = [1., 2.]
y = [1., 2.]
start = time.time()
fig, axs = plt.subplots(10, 10)
axs = axs.flatten()
for ax in axs:
ax.plot(x, y )
stop = time.time()
print(stop - start) I get 2.89 s w/o this change and 0.95 with the change. If I don't plot I get just a bit faster: Like its great this speeds things up so much, but why the heck is the initial tick instantiation so slow? The plotting just adds 0.1 s to the whole thing. |
This should go against master. Tried to re-target it and there are conflicts, I'll do the 2.1.x -> master merge and try again. Unfortunately many parts of our API that should be private are not and users have found almost all of the API surface... This will just need an api_changes note with any mitigation steps (sounds like just calling That is a crazy speed up! Thanks for working on this @timhoffm ! |
I think this is really clever and it is great that it speeds things up. OTOH I feel it is a complex solution to the fact that we can't find the recursion in |
successfully re-targetted to master. |
On 2017/11/10 4:59 AM, Jody Klymak wrote:
I think this is really clever and it is great that it speeds things up.
OTOH I feel it is a complex solution to the fact that we can't find the
recursion in |reset_ticks| in our already complex code.
We have found the recursion; @QuLogic and I both produced PRs that
handle the problem that way, but in both cases it turns out that there
are problems when applying our solutions to current master, and
questions about consequences for custom Axes subclasses.
We might still want to include some variation of our attempts to stop
the redundant cla calls, but even if we do I think this lazy
initialization will be beneficial--it will provide added speedup in
cases where no ticks are ever used.
Longer term, perhaps a major re-engineering can be done to simplify the
whole over-complex and fragmented tick and tick label system, including
the locator and formatter system.
|
5901e0e
to
3288b01
Compare
3288b01
to
f494913
Compare
I've followed the suggestion of @anntzer to make this a private class @jklymak I've looked at Also I've figured, that @QuLogic Thanks for targetting From my point of view, this is all I want to do with this pull requests. So it's ready to merge. Comments are welcome. |
As @tacaswell commented above, this needs an api_changes note because |
b28ad0e
to
77fa33f
Compare
@efiring @tacaswell I've add the api_changes. Speaking of api changes, should we make If you don't want to break this immediately, one could at least change it internally and add a property that restores the original behavior but issues a deprecation warning. |
@QuLogic, what are you thoughts on this now? |
can be called multiple times without the overhead of initialzing every | ||
time. | ||
|
||
https://github.com/matplotlib/matplotlib/issues/6664 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to consider that checking the git blame is sufficient to see where this comes from, and prefer not adding explicit links to issues (otherwise it's a bit endless). Not a huge deal of course.
@@ -1339,22 +1374,15 @@ def get_major_ticks(self, numticks=None): | ||
numticks = len(self.get_major_locator()()) | ||
if len(self.majorTicks) < numticks: | ||
# update the new tick label properties from the old | ||
protoTick = self.majorTicks[0] | ||
for i in range(numticks - len(self.majorTicks)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could even just replace the outer "if len(self.majorTicks) < numticks" by "while len() < numticks" and remove one indent level.
PR Summary
Improves #6664 by lazily initializing the axis ticks.
Implementation details
Instead of initializing
self.majorTicks
andself.minorTicks
to 1-element default lists, we useLazyDefaultTickList
as a proxy, that instantiates the default list and replaces itself with it on access.This is almost a drop-in replacement. The only thing I didn't get to work transparently is mimicking concatenation. Where
self.majorTicks + self.minorTicks
works for lists, it does not for theLazyDefaultTickList
. There's one occurence of this in the code. It could be replaced bylist(self.majorTicks) + list(self.minorTicks)
, but I chose to rewrite this using iteratorI assume that
self.majorTicks
andself.minorTicks
are considered private (they do not show in the docs) and that they should only be modified using the methods ofAxis
. Therefore, I think it's ok thatself.majorTicks + self.minorTicks
does not work withLazyDefaultTickList
.Performace Gain
Instatiation of a plot with many axes has become faster by a factor of three.
before change:

after change:

As a nice side effect, the test suite runs 10% faster after the change.
Tests
Since this is an drop-in replacement, I don't think that it needs extra tests. I ran the test suite and saw no difference. Before and after the change I had 4 failing tests, so I suppose their failure is unrelated.
PR Checklist
Other checklist items do not apply.