Svg rasterize resolution fix #1185

DaMichel · 2012-09-01T07:48:25Z

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.

…lows numpy.float32 in pdfRepr

pelson · 2012-09-01T09:34:14Z

lib/matplotlib/backends/backend_svg.py

@@ -733,6 +734,11 @@ def draw_gouraud_triangles(self, gc, triangles_array, colors_array,
    def option_scale_image(self):
        return True



Nitpick: Only need a single new line between methods.

pelson · 2012-09-01T09:46:01Z

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.

dmcdougall · 2012-09-20T15:59:57Z

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?

mdboom · 2012-09-20T16:05:42Z

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

dmcdougall · 2012-09-20T16:13:59Z

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

dmcdougall · 2012-09-20T19:54:03Z

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)):


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

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

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

yarikoptic · 2012-10-08T20:08:37Z

@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

WeatherGod · 2012-10-08T20:14:36Z

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):


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

This API change still needs to be addressed.

yarikoptic · 2012-10-08T20:26:28Z

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

mdboom · 2012-10-08T20:28:33Z

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.

yarikoptic · 2012-10-08T20:35:29Z

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 · 2012-10-09T12:09:20Z

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)

dmcdougall · 2012-11-07T21:11:48Z

Besides a rebase, is anything else needed for this?

tbekolay · 2013-02-22T03:16:21Z

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.

WeatherGod · 2013-02-22T14:04:35Z

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)


API change needs to be fixed here

DaMichel · 2013-05-03T08:02:03Z

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.

mdboom · 2013-05-03T13:30:01Z

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.

mdboom · 2013-05-03T13:35:32Z

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 · 2013-05-04T10:52:01Z

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 :)

pelson · 2013-05-07T11:15:46Z

lib/matplotlib/backends/backend_svg.py

@@ -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):


@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 😉

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

pelson · 2013-05-07T11:18:49Z

@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 😄

pelson · 2013-05-07T13:02:23Z

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

mdboom · 2013-05-07T14:05:45Z

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

pelson · 2013-05-07T15:12:29Z

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.

pelson · 2013-05-13T15:07:14Z

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

DaMichel · 2013-05-14T07:55:09Z

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

DaMichel · 2013-05-17T10:08:09Z

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.

pwuertz · 2013-05-22T03:22:39Z

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

mdboom · 2013-05-22T15:16:17Z

@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?

pwuertz · 2013-05-22T16:48:29Z

Indeed, no worries then ;)

mdboom · 2013-05-24T19:26:50Z

Closed by #2044.

Michael Welter added 2 commits August 26, 2012 10:10

fixed: compare_images for older numpy versions. added: backend_pdf al…

309c93c

…lows numpy.float32 in pdfRepr

fix: dpi setting should work now for rasterization in svg backend

cb1aa37

pelson reviewed Sep 1, 2012
View reviewed changes

dmcdougall reviewed Sep 20, 2012
View reviewed changes

Michael Welter added 3 commits October 3, 2012 18:26

update for my patch

868f411

update for my patch

98ab64e

update for my patch

7eacb85

DaMichel closed this Oct 3, 2012

WeatherGod reviewed Oct 8, 2012
View reviewed changes

DaMichel reopened this Oct 9, 2012

WeatherGod reviewed Feb 22, 2013
View reviewed changes

api change

f56a455

pelson reviewed May 7, 2013
View reviewed changes

pelson mentioned this pull request May 7, 2013

MixedModeRenderer non-72-dpi fixes & Pgf mixed rendering #1975

Merged

Michael Welter added 3 commits May 17, 2013 10:04

added test result images

c610462

added note to whats_new.rst

dc0a819

added a note to changelog

d6f4931

mdboom mentioned this pull request May 21, 2013

Svg rasterize (rebased) #2044

Merged

mdboom closed this May 24, 2013

		@@ -733,6 +734,11 @@ def draw_gouraud_triangles(self, gc, triangles_array, colors_array,
		def option_scale_image(self):
		return True

Svg rasterize resolution fix #1185

Svg rasterize resolution fix #1185

Conversation

DaMichel commented Sep 1, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pelson commented Sep 1, 2012

dmcdougall commented Sep 20, 2012

mdboom commented Sep 20, 2012

dmcdougall commented Sep 20, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yarikoptic commented Oct 8, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yarikoptic commented Oct 8, 2012

mdboom commented Oct 8, 2012

yarikoptic commented Oct 8, 2012

DaMichel commented Oct 9, 2012

dmcdougall commented Nov 7, 2012

tbekolay commented Feb 22, 2013

Choose a reason for hiding this comment

DaMichel commented May 3, 2013

mdboom commented May 3, 2013

mdboom commented May 3, 2013

DaMichel commented May 4, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pelson commented May 7, 2013

pelson commented May 7, 2013

mdboom commented May 7, 2013

pelson commented May 7, 2013

pelson commented May 13, 2013

DaMichel commented May 14, 2013

DaMichel commented May 17, 2013

pwuertz commented May 22, 2013

mdboom commented May 22, 2013

pwuertz commented May 22, 2013

mdboom commented May 24, 2013