Svg rasterize resolution fix #1185

Closed
wants to merge 9 commits into
from

Conversation

Projects
None yet
10 participants

DaMichel commented Sep 1, 2012

i made a patch which would allow the svg backend to make rasterized plots according to the dpi given in savefig. I wanted this in order to have high-res scatter plots. As you probably know it is hardcoded to 72 dpi. The idea how to, came from the pdf backend. I mostly copy pasted some code.

I also added a test for it. Some other tests now fail the image comparison. It is nothing serious apparently. Probably roundoff errors. Except interp_nearest_vs_none. The result of which looks now exactly like the pdf which it did not before.

Also, two other minor changes which i needed

  • Compare_images failed for older numy versions. Simple fix ...
  • A numpy.float32 variable got passed to the pdfRepr function in
    backend_pdf.py. I changed this function to accept this type, too.
@@ -733,6 +734,11 @@ def draw_gouraud_triangles(self, gc, triangles_array, colors_array,
def option_scale_image(self):
return True
@pelson

pelson Sep 1, 2012

Member

Nitpick: Only need a single new line between methods.

@@ -1113,25 +1130,17 @@ def print_svgz(self, filename, *args, **kwargs):
def _print_svg(self, filename, svgwriter, fh_to_close=None, **kwargs):
try:
+ image_dpi = kwargs.pop("dpi", 72)
@pelson

pelson Sep 1, 2012

Member

What's the difference between this image_dpi and self.image_dpi?

@DaMichel

DaMichel Oct 3, 2012

This code is in a different class. From here image_dpi gets passed to the backends constructor.

lib/matplotlib/testing/compare.py
@@ -308,8 +308,8 @@ def compare_images( expected, actual, tol, in_decorator=False ):
h1p = expectedImage[:,:,i]
h2p = actualImage[:,:,i]
- h1h = np.histogram(h1p, bins=bins)[0]
- h2h = np.histogram(h2p, bins=bins)[0]
+ h1h = np.histogram(h1p, bins=ns)[0]
@pelson

pelson Sep 1, 2012

Member

Pretty sure this has been fixed by another PR in a different place (ns became bins). Will look for the PR #.

lib/matplotlib/tests/test_image.py
@@ -160,6 +160,33 @@ def test_no_interpolation_origin():
ax = fig.add_subplot(212)
ax.imshow(np.arange(100).reshape((2, 50)), interpolation='none')
+
+@image_comparison(baseline_images=['rasterize_10dpi'], extensions=['pdf','svg'], tol=1.5e-3, remove_text=True)
@pelson

pelson Sep 1, 2012

Member

Line width is probably longer than 80 chars. Please wrap the line.

lib/matplotlib/tests/test_image.py
+ # This test should check rasterized rendering with high output resolution.
+ # It plots a rasterized line and a normal image with implot. So it will catch
+ # when images end up in the wrong place in case of non-standard dpi setting.
+ # Instead of high-res rasterization i use low-res. Therefor the fact that the
@pelson

pelson Sep 1, 2012

Member

Typo. Therefor -> Therefore. (if your interested, therefor is a proper word, but would not be correct in this case: http://walkinthewords.blogspot.co.uk/2009/04/therefor-vs-therefore.html)

+ axes[2].plot([0,1],[0,1], linewidth=20.)
+ axes[2].set(xlim = (0,1), ylim = (-1, 2))
+
+ rcParams['savefig.dpi'] = 10
@pelson

pelson Sep 1, 2012

Member

Unless I am mistaken, this is going to have a side effect on subsequent tests. Perhaps the image comparison decorator should accept a dictionary of overwriting rc params...

@DaMichel

DaMichel Oct 3, 2012

Oddly enough it actually doesn't show any side effects. It would surely show in the images. Some good old debugging through print statements also shows that apparently changes to rcParams don't carry over to the next test.
Correct me if i am wrong though. My guess is that the testing framework reloads everything from scratch for each test.

@DaMichel

DaMichel Oct 9, 2012

p.s. we already have

@image_comparison(baseline_images=['interp_nearest_vs_none'],
                  extensions=['pdf', 'svg'], remove_text=True)
def test_interp_nearest_vs_none():
    'Test the effect of "nearest" and "none" interpolation'
    # Setting dpi to something really small makes the difference very
    # visible. This works fine with pdf, since the dpi setting doesn't
    # affect anything but images, but the agg output becomes unusably
    # small.
    rcParams['savefig.dpi'] = 3
    X = np.array([[[218, 165, 32], [122, 103, 238]],
                  [[127, 255, 0], [255, 99, 71]]], dtype=np.uint8)
    fig = plt.figure()
    ax1 = fig.add_subplot(121)
    ax1.imshow(X, interpolation='none')
    ax1.set_title('interpolation none')
    ax2 = fig.add_subplot(122)
    ax2.imshow(X, interpolation='nearest')
    ax2.set_title('interpolation nearest')

So, hopefully everything is alright.

@pelson

pelson May 7, 2013

Member

Thanks @mwelter - I was mistaken 😄

Member

pelson commented Sep 1, 2012

Looks like a sensible extension of the svg capabilities to me. This has missed the 1.2.x freeze, and my feeling is that this is a nice-to-have that we don't need to fast-track; I propose that we wait to get this into the 1.3 release.

Member

dmcdougall commented Sep 20, 2012

This will, presumably, also need an entry in api_changes.rst. Unless I'm missing something?

Since there is now a v1.2.x branch, is it safe to get the ball rolling on this? Or should we wait until the actual 1.2 release to avoid possible merge conflicts?

Owner

mdboom commented Sep 20, 2012

It's fine to merge non-1.2.x changes into master.

Member

dmcdougall commented Sep 20, 2012

@mdboom Cheers for the clarification.

IMHO this is not ready to merge until at least @pelson's issues have been addressed. Also, I take back my comment about needing an entry to api_changes.rst. I think you only need to add an entry there if the user sees the API change. I could be mistaken, however.

lib/matplotlib/backends/backend_pdf.py
@@ -139,7 +139,7 @@ def pdfRepr(obj):
# Floats. PDF does not have exponential notation (1.0e-10) so we
# need to use %f with some precision. Perhaps the precision
# should adapt to the magnitude of the number?
- elif isinstance(obj, float):
+ elif isinstance(obj, (float, np.float32)):
@dmcdougall

dmcdougall Sep 20, 2012

Member

Is there any reason why we shouldn't use cbook.is_numlike and cbook.is_scalar?

@efiring

efiring Sep 20, 2012

Owner

@dmcdougall Why would you want to use cbook.is_numlike or is_scalar here? In any case, it looks like a change to the line in question has already been made by another PR.

@mwelter, I don't think this PR should be touchiung backend_pdf at all.

@dmcdougall

dmcdougall Sep 20, 2012

Member

@efiring Hmmm. I thought a numpy version independent way of checking whether something was a number would be neater. I guess we do actually want floating point numbers here. I can't find a cbook.is_floatlike, so this way is probably better, you're right.

@DaMichel DaMichel closed this Oct 3, 2012

@mwelter -- could you please explain why this PR was just closed after all?

I have ran into this limitation as well, searched the ML to finally get into this PR... but then saw it closed and found no reflection of it in GIT, so looks like the idea was totally abandoned. Is that so? (then I would need to file a new bug report I guess describing this precise issue)

Thanks in advance for explanations

lib/matplotlib/backends/backend_svg.py
@@ -244,10 +244,11 @@ class RendererSVG(RendererBase):
FONT_SCALE = 100.0
fontd = maxdict(50)
- def __init__(self, width, height, svgwriter, basename=None):
+ def __init__(self, width, height, image_dpi, svgwriter, basename=None):
@WeatherGod

WeatherGod Oct 8, 2012

Member

Just noticed this. This changes the API of this class. Backends are used by other projects and are not considered to be internal-only, IMO. It would be better to put image_dpi at the end of the call signature with a None (or whatever default value that makes sense).

@WeatherGod

WeatherGod Feb 22, 2013

Member

This API change still needs to be addressed.

I guess in the long run (?), it might be altogether worth unifying API of backends:

$> grep -A30 '(RendererBase)' *py | grep -e '\((RendererBase):\|__init__\)'
backend_agg.py:class RendererAgg(RendererBase):
backend_agg.py-    def __init__(self, width, height, dpi):
backend_agg.py-        if __debug__: verbose.report('RendererAgg.__init__', 'debug-annoying')
backend_agg.py-        RendererBase.__init__(self)
backend_agg.py-        if __debug__: verbose.report('RendererAgg.__init__ width=%s, height=%s'%(width, height), 'debug-annoying')
backend_cairo.py:class RendererCairo(RendererBase):
backend_cairo.py-    def __init__(self, dpi):
backend_emf.py:class RendererEMF(RendererBase):
backend_emf.py-    def __init__(self, outfile, width, height, dpi):
backend_gdk.py:class RendererGDK(RendererBase):
backend_gdk.py-    def __init__(self, gtkDA, dpi):
backend_macosx.py:class RendererMac(RendererBase):
backend_macosx.py-    def __init__(self, dpi, width, height):
backend_pdf.py:class RendererPdf(RendererBase):
backend_pdf.py-    def __init__(self, file, image_dpi):
backend_pdf.py-        RendererBase.__init__(self)
backend_pgf.py:class RendererPgf(RendererBase):
backend_pgf.py-    def __init__(self, figure, fh):
backend_ps.py:class RendererPS(RendererBase):
backend_ps.py-    def __init__(self, width, height, pswriter, imagedpi=72):
backend_svg.py:class RendererSVG(RendererBase):
backend_svg.py-    def __init__(self, width, height, svgwriter, basename=None):
backend_template.py:class RendererTemplate(RendererBase):
backend_template.py-    def __init__(self, dpi):
backend_wx.py:class RendererWx(RendererBase):

Not sure if my grep caught right ones but I guess it did... since there seems to be no agreement at all, imho they all should be keyword arguments for consistent digestion (actually dpi seems to be the most consistently present ;) )... but I can be utterly wrong

Owner

mdboom commented Oct 8, 2012

Since each of the backends may need entirely different objects to construct it's renderer, there's not much benefit in having consistent constructor signatures, other than least surprise. Obviously the methods on the instances created can and should be consistent between backends, however.

So is that the issue of simply moving dpi into a keyword argument? If
@mwelter is busy, I bet even I could do that if necessary (probably
still preferably at least pointing toward some unification, i.e. making
it just a 'dpi=' kwarg). Just let me know ;)

Yaroslav O. Halchenko
Postdoctoral Fellow, Department of Psychological and Brain Sciences
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
Phone: +1 (603) 646-9834 Fax: +1 (603) 646-1419
WWW: http://www.linkedin.com/in/yarik

@DaMichel DaMichel reopened this Oct 9, 2012

DaMichel commented Oct 9, 2012

Sorry for the confusion. I closed the PR on accidentally.

I think the PR is actually ready now as far as it concerns the earlier issues.
Except that i am not sure about #1185 (comment)

Member

dmcdougall commented Nov 7, 2012

Besides a rebase, is anything else needed for this?

Contributor

tbekolay commented Feb 22, 2013

Thanks to @mwelter! I just merged this fix locally and it worked great for me. SVGs look much better now. It would be great if this were merged into master so others can get this fix.

lib/matplotlib/backends/backend_svg.py
self.figure.set_dpi(72.0)
width, height = self.figure.get_size_inches()
w, h = width*72, height*72
if rcParams['svg.image_noscale']:
- renderer = RendererSVG(w, h, svgwriter, filename)
+ renderer = RendererSVG(w, h, image_dpi, svgwriter, filename)
@WeatherGod

WeatherGod Feb 22, 2013

Member

API change needs to be fixed here

DaMichel commented May 3, 2013

Is it broken?

One way or the other, RendererSVG needs to know the DPI in which it should render images. I'm at a loss with how else i could provide this information.

Prior to my patch this was not an issue since one could just work with 72 DPI and 72 DPI is still hardcode in quite a few places in RendererSVG. For the purpose of rasterization, the function get_image_magnification provides a scaling factor to the component which does the actual rendering of the rasterized data.
Bear in mind that my patched code is analogous to the way in which the pdf backbone deals with the resolution thing.

Owner

mdboom commented May 3, 2013

Firstly, sorry this patch has sat here for so long. Personally, I'm never offended if the author of a patch takes the initiative to ping those of us with commit rights when things are ready. We've got a lot of issues, and far less time, so things do get lost 😉.

I think @WeatherGod is just commenting that the API has changed. Instead, I think the RendererSVG signature should be:

def __init__(self, width, height, svgwriter, basename=None, image_dpi=72):

That way, code written for the old signature will continue to work.

Other than that, I would love to see a test for this. I don't think an image comparison test would be helpful, but I think one could just write out figure containing an image to an SVG to a io.StringIO object, setting the image_dpi to, say, 300, and then make sure that the file size is larger than one would expect if the image were rendered at only 72 dpi. I'll go ahead and draft up what I have in mind. Expect a PR against your branch shortly.

Owner

mdboom commented May 3, 2013

Ugh -- I haven't had enough coffee yet this morning. I see you already have a test, and it looks sufficient to catch the case where this stops working.

I think we're good to go, once the minor API issue is resolved, but this does need a rebase against master. 👍 from me.

DaMichel commented May 4, 2013

Perfect. I changed the signature.
No problem with having this PR open for some time but i am nonetheless glad when it is finally pulled :)

@@ -733,6 +734,9 @@ def draw_gouraud_triangles(self, gc, triangles_array, colors_array,
def option_scale_image(self):
return True
+ def get_image_magnification(self):
+ return self.image_dpi/72.0
+
def draw_image(self, gc, x, y, im, dx=None, dy=None, transform=None):
@pelson

pelson May 7, 2013

Member

@mwelter - I don't know how much you've invested in checking out the details of this signature but it is worth noting that all backends have it, except for the backend_bases.py superclass. Would you be willing to submit a PR which adds the necessary signature updates to backend_bases.py along with the necessary keyword documentation?

I promise that PR wont take 8 months to merge 😉

@DaMichel

DaMichel May 17, 2013

Nah, sorry. I happly contribute what i have done. But at the moment i am not willing to look into more stuff.

Member

pelson commented May 7, 2013

@mwelter - the tests results need adding to the PR.
Would you also mind adding a short entry in the doc/users/whats_new.rst section?

After that, I think this is good to go. I'm really sorry its taken 8 months! The good news is that this will be available shortly in the v1.3 release 😄

Member

pelson commented May 7, 2013

Does a similar this also exist for the Agg backend? I couldn't find a way to control the rasterization dpi in Agg...

Owner

mdboom commented May 7, 2013

Since Agg is a rasterizer, images are always rendered at native resolution.

Member

pelson commented May 7, 2013

Since Agg is a rasterizer, images are always rendered at native resolution.

Ah, shame - I wanted to rasterize to increase a resolution of a rasterized Artist (to reproduce AA artefacts). Never mind.

Member

pelson commented May 13, 2013

@mwelter - the tests results need adding to the PR.
Would you also mind adding a short entry in the doc/users/whats_new.rst section?

Nudge. @mwelter - are you in a position to add these things? The v1.3.x release is getting close...

Yes. Should be doable till the end of the week.

All right i added the test images and some documentation. Beware that here the patch leads to some failures in other tests:

  • test_axes.test_hist2d

    test_axes.test_imshow

    test_image.test_image_interps

    test_image.test_imshow

Just due to roundoff errors i guess. Pixels along the rim of objects have changed just enough to trigger a failure.

  • test_image.test_interp_nearest_vs_none: Now there is an actual difference between interpolation types. Previously both side were the same!
  • test_image.test_image_clip: Now looks like clip-expected.png, but compared to previous clip-expected.svg the text locations has changed a little bit.

I did not add updated versions of these test results. I guess new images are best added by the maintainer because i still use an old code base to not mess up the branches and things might have changed.

@mdboom mdboom referenced this pull request May 21, 2013

Merged

Svg rasterize (rebased) #2044

Contributor

pwuertz commented May 22, 2013

Hm, introducing a new image_dpi property looks like a workaround for not being able to change the figure dpi from 72 to anything else (I'm trying to solve that in #1975). @pelson suggested to take a closer look at this after merging the svg rasterize fix but maybe it's better to do it the other way around. The image_dpi property and image magnification functions might vanish in the process..

Owner

mdboom commented May 22, 2013

@pwuertz: My understanding is that the image_dpi property is still constructed from the dpi kwarg to savefig, so from the outside it looks the same. Or am I missing something?

Contributor

pwuertz commented May 22, 2013

Indeed, no worries then ;)

Owner

mdboom commented May 24, 2013

Closed by #2044.

@mdboom mdboom closed this May 24, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment