Skip to content
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

im caching in draw_tex #796

Closed
jdh2358 opened this issue Mar 24, 2012 · 5 comments · Fixed by #4721
Closed

im caching in draw_tex #796

jdh2358 opened this issue Mar 24, 2012 · 5 comments · Fixed by #4721

Comments

@jdh2358
Copy link
Collaborator

jdh2358 commented Mar 24, 2012

This method is really confusing me (https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/backends/backend_agg.py#L199):

def draw_tex(self, gc, x, y, s, prop, angle):
    # todo, handle props, angle, origins
    size = prop.get_size_in_points()

    texmanager = self.get_texmanager()
    key = s, size, self.dpi, angle, texmanager.get_font_config()
    im = self.texd.get(key)
    if im is None:
        Z = texmanager.get_grey(s, size, self.dpi)
        Z = np.array(Z * 255.0, np.uint8)

    self._renderer.draw_text_image(Z, x, y, angle, gc)

Specifically, if "im" is not None after the texd.get call, then "Z" is not defined in the draw_text_image call. That we are not seeing failures suggests the im is never getting cached and perhaps we should just remove this logic.

@efiring
Copy link
Member

efiring commented Oct 13, 2013

@mdboom, John's comment is still valid, and applies to macosx as well. What do you think: should the caching be implemented, or should the cache-related code be deleted? The deletion looks trivial, but I don't know what would be required to make the caching work. I suspect that would be obvious to you, though.

@mdboom
Copy link
Member

mdboom commented Oct 14, 2013

This must have just been accidentally disengaged at some point. The caching is pretty trivial to implement (we use this pattern in a number of other places):

diff --git a/lib/matplotlib/backends/backend_agg.py b/lib/matplotlib/backends/backend_agg.py
index 3ba1e57..dc9534a 100644
--- a/lib/matplotlib/backends/backend_agg.py
+++ b/lib/matplotlib/backends/backend_agg.py
@@ -234,7 +234,8 @@ class RendererAgg(RendererBase):
         im = self.texd.get(key)
         if im is None:
             Z = texmanager.get_grey(s, size, self.dpi)
-            Z = np.array(Z * 255.0, np.uint8)
+            im = np.array(Z * 255.0, np.uint8)
+            self.texd[key] = im

         w, h, d = self.get_text_width_height_descent(s, prop, ismath)
         xd = d * np.sin(np.deg2rad(angle))
@@ -242,7 +243,7 @@ class RendererAgg(RendererBase):
         x = np.round(x + xd)
         y = np.round(y + yd)

-        self._renderer.draw_text_image(Z, x, y, angle, gc)
+        self._renderer.draw_text_image(im, x, y, angle, gc)

     def get_canvas_width_height(self):
         'return the canvas width and height in display coords'

However, I don't know if it matters a whole lot. I wrote a quick benchmark that renders the same figure 500 times where the text doesn't change (which is often the case in many animations -- obviously the case where the text is changing a lot do not benefit from the cache). I actually couldn't measure any significant change on repeated runs (other things are really dominant). The margin of error was such that some runs with cache were slower than runs without cache. I'm actually very surprised by the result, so please double check my work. (This is on Linux. It might matter more on platforms where creating processes was more expensive.)

Here's the benchmark:

import matplotlib
matplotlib.use("Agg")
matplotlib.rcParams["text.usetex"] = True

from pylab import *

t = arange(0.0, 1.0+0.001, 0.001)
s = cos(2*2*pi*t)
plot(t, s, '-', lw=2)

xlabel('time (s)')
ylabel('voltage (mV)')
title('About as simple as it gets, folks')

import time

frames = 500.0
t = time.time()
c = time.clock()
for i in range(int(frames)):
    gcf().canvas.draw()
wallclock = time.time() - t
user = time.clock() - c
print ("wallclock:", wallclock)
print ("user:", user)
print ("fps:", frames / wallclock)

So, in favor of simplicity, I guess I'm in favor of removing the cache, unless I've made an error in the benchmarking here.

@pelson
Copy link
Member

pelson commented Oct 18, 2013

The moral of the story is that you should never implement optimisations unless you've measured the problem in the first place. Python often throws up surprising results with regards to performance, and more often than not I've found my own intuition is wrong. I haven't verified your results, but matplotlib doesn't currently have the caching working, and there is no clear benefit to implementing is, so lets take this opportunity to simplify this tiny bit of code, and just delete the if statement.

Smiles all round! Thanks for bringing this up again @efiring.

@mdboom
Copy link
Member

mdboom commented Oct 18, 2013

It's also important to remember that this code goes back 9-10 years. It may have mattered then, and doesn't now. It's quite unlikely that John (who I think wrote this) was using a quad core machine with an SSD as I am today (both of which probably matter quite a bit for spawning large subprocesses).

@tacaswell tacaswell added this to the v1.4.x milestone Aug 18, 2014
@tacaswell tacaswell modified the milestones: v1.4.x, 1.5.0 Feb 7, 2015
@tacaswell
Copy link
Member

tacaswell added a commit to tacaswell/matplotlib that referenced this issue Jul 17, 2015
@tacaswell tacaswell self-assigned this Jul 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants