PGF Backend: Support interpolation='none'

[f0k]

As discussed in #6740, the PGF backend currently doesn't support interpolation='none', which means all raster graphics are interpolated prior to saving them. This adds full support for affine image transforms.

In the old backend, it took me 10 minutes to implement this from what I already had in #6740: https://gist.github.com/f0k/f8c87ffd82488c2ed303989592bf4ebd/eeaa54c8b98f84fd2f80caf4305034fc1eeeb76f#file-backend_mypgf-py-L61-L62
For the new backend, things seem to have changed quite a bit, but documentation is still lacking. It took me an hour of fiddling around. Now the code produces the same result as the PDF backend for the affine transform demo in its current version, which does not include the skewing test any more.

Closes #6740.

[f0k]

Now the code produces the same result as the PDF backend for the affine transform demo in its current version, which does not include the skewing test any more.

Re-added the skewing test to the demo and two more transformations. Images and dashed rectangle targets match up nicely for all transforms both in the PDF and PGF backend. Ready to merge from my side.

[afvincent]

Shouldn't a plt.show be added at the end of the demo_affine_image.py?

Actually, I rather prefer your example to what I wrote in #6673 :). Small remark: it seems that you could remove l. 33 x3, y3 = x2, y1 as it doesn't seem to be used anywhere (to be honest, @QuLogic did the same remark about my PR).

Besides, it looks like the weird offset between the image and extent rectangle (see #6673) is still present when I plot the new version of the example:

(NB: I change the color of the extent rectangle to red)
And this is even worse when exporting to PDF: demo_affine_PR6792.pdf

@tacaswell Should I open a dedicated issue for the latter odd behavior, as it tends to deviate from the original PR?

[f0k]

Shouldn't a plt.show be added at the end of the demo_affine_image.py?

I don't know, it was removed by @mdboom in 1384c7c. You tell me :)

Small remark: it seems that you could remove l. 33 x3, y3 = x2, y1 as it doesn't seem to be used anywhere

Oh, didn't notice. Will amend my commit.

Besides, it looks like the weird offset between the image and extent rectangle (see #6673) is still present when I plot the new version of the example

That's because the rectangle lines are centered on the left and bottom pixel boundaries rather than the pixel centers. This is consistent within matplotlib. The way the image extents are currently chosen ([-2, 4, -3, 2]), it's a hassle to offset the rectangle by half a pixel, but we could do so.
/edit: Hmm, no, the offset is less than half a pixel of that image. Sorry, that was a red herring.

(NB: I change the color of the extent rectangle to red)

I think red doesn't look nice with the new default colormap, that's why I changed it from blue (the new default color for the plot) to yellow. I'd be fine either way.

[f0k]

Oh, didn't notice. Will amend my commit.

Amended my comment to remove the unused x3, y3 declaration and re-add the plt.show() at the end.

[afvincent]

@f0k Thank you for amending the example! I will then close my redundant PR #6673.

About the red color: I only used it for me. I agree that blue was a bit difficult to see, and if you prefer to replace it with yellow, it's fine for me.

About the offset between the extent rectangle and the image, if it's normal behavior, then everything is alright (I was just scared it might be a small bug), and indeed it's unclear to me that it's really worth the hassle to offset the rectangle by half pixel.

For the PGF backend part, I apologize but I don't know enough about backends to review it.

[f0k]

About the offset between the extent rectangle and the image, if it's normal behavior, then everything is alright (I was just scared it might be a small bug)

I retracted this, I'm not sure it should be this way, but I guess the problem lies outside the demo code. It's not the business of our PRs then.

I will then close my redundant PR #6673.

I liked the outline showing the rotation, we could think about adding this here as well (does it make sense for the other transformations?). But probably this should still be a separate PR -- I extended the demo merely to check whether the modified PGF backend works correctly, not to make the demo more instructive.

For the PGF backend part, I apologize but I don't know enough about backends to review it.

Let's try to summon @pwuertz for this! (He's the original author of the PGF backend, not sure how much he's still involved in matplotlib.)

[pwuertz]

Ok, reviewed the changes to the PGF backend.

One small detail (just an opinion, feel free to ignore it if you don't agree):
Although merging the (x, y) base translation into the transformation matrix is obviously the cleverest thing to do I'd vote for not doing it. The (x=0, y=0) coordinates must still be written to the pgf command, there is no computational gain from modifying them, just more code in the transform != None path. Also it might be easier to debug the pgf output later if you can tell apart the (x, y) and transform parameters the way they appear within matplitlib too.
I'd just remove lines 647--650 for general simplification.

My main concern before was the half-pixel/pixel alignment, but if your extended demo indeed shows that this is a matplotlib-wide issue and not a backend-to-backend inconsistency it shouldn't hold up a merge of this PR. Better alignment is a job for another day then.

In conclusion: Nice work! Thanks for tracking down the details for making this possible.

[f0k]

Although merging the (x, y) base translation into the transformation matrix is obviously the cleverest thing to do I'd vote for not doing it.

This was not done for cleverness. If we remove lines 647--650, the translation is applied before the affine transform. This was correct for matplotlib 1.5, but is wrong for matplotlib 2.0. Integrating it in the transformation seemed easier than moving the \pgftext one level out (as I was unsure whether this would mess up anything). Should we try that?

My main concern before was the half-pixel/pixel alignment, but if your extended demo indeed shows that this is a matplotlib-wide issue and not a backend-to-backend inconsistency it shouldn't hold up a merge of this PR.

It's definitely consistent between backends.

[f0k]

@pwuertz: Moved \pgftext to the outside so the translation is applied after the affine transform (and not merged into it any longer). Also removed \makeatletter and \makeatother since there's a \makeatletter in the beginning of the PGF output. And finally, I slightly changed the code such that the image is output with height and width of 1.0 inches instead of 1.0/dpi inches, and the affine transform is adapted accordingly. Seems nicer to me.

I can merge this into the second commit before merging if needed, I just separated it for now so @pwuertz can have a look.

[pwuertz]

Ok, the code looks really good now.

Unfortunately the new image handling is causing trouble on my machine. When I save the demo figure to PDF using the modified backend_pgf the images are missing completely (Xelatex from Ubuntu 16.04 Texlive).

How did you set up your tests so far? Pdf, xe, lualatex? Version? Os? Tex distribution?

[f0k]

When I save the demo figure to PDF using the modified backend_pgf the images are missing completely

Booh. Thanks for testing!

How did you set up your tests so far? Pdf, xe, lualatex? Version? Os? Tex distribution?

Ubuntu 14.04, pdflatex (pdfTeX 3.1415926-2.5-1.40.14 (TeX Live 2013/Debian)). I think pdflatex translates the \pgfsys@transformcm directly into a cm (concatenate matrix) operator in the PDF output.
If you change return True to return False in the option_scale_image() method, does everything still look okay?
If you go back to be54fae, does anything change?

[pwuertz]

I'm seeing good results with pdflatex and lualatex, only xelatex fails. Your initial version with the translation merged into the matrix works however. Might be a bug in xelatex, but I guess it's not worth figuring it out and instead use your first approach again.

[tacaswell]

@f0k A doc PR to write down your hard won (sorry) understanding of how the image handling works would be appreciated!

[pwuertz]

@f0k This block here is based on your last commit and works with all 3 texsystems.

# reference the image in the pgf picture
writeln(self.fh, r"\begin{pgfscope}")
self._print_pgf_clip(gc)
f = 1. / self.dpi  # from display coords to inch
writeln(self.fh, r"\pgfsys@transformshift{%fin}{%fin}" % (x * f, y * f))
if transform is not None:
    tr1, tr2, tr3, tr4, tr5, tr6 = transform.frozen().to_values()
    w = h = 1. / f  # scale is already included in the transform
    writeln(self.fh, r"\pgfsys@transformcm{%f}{%f}{%f}{%f}{%fin}{%fin}" % (tr1 * f, tr2 * f, tr3 * f, tr4 * f, tr5 * f, tr6 * f))
interp = str(transform is None).lower()  # interpolation in PDF reader
writeln(self.fh, r"\pgftext[left,bottom]{\pgfimage[interpolate=%s,width=%fin,height=%fin]{%s}}" % (interp, w * f, h * f, fname_img))
writeln(self.fh, r"\end{pgfscope}")

[f0k]

This block here is based on your last commit and works with all 3 texsystems.

Great, thanks for figuring it out! Shall we really use both transformshift and transformcm at the same time, or change to:

# reference the image in the pgf picture
writeln(self.fh, r"\begin{pgfscope}")
self._print_pgf_clip(gc)
f = 1. / self.dpi  # from display coords to inch
if transform is None:
    writeln(self.fh, r"\pgfsys@transformshift{%fin}{%fin}" % (x * f, y * f))
    w, h = w * f, h * f
else:
    tr1, tr2, tr3, tr4, tr5, tr6 = transform.frozen().to_values()
    writeln(self.fh, r"\pgfsys@transformcm{%f}{%f}{%f}{%f}{%fin}{%fin}" % (tr1 * f, tr2 * f, tr3 * f, tr4 * f, (tr5 + x) * f, (tr6 + y) * f))
    w = h = 1  # scale is already included in the transform
interp = str(transform is None).lower()  # interpolation in PDF reader
writeln(self.fh, r"\pgftext[left,bottom]{\pgfimage[interpolate=%s,width=%fin,height=%fin]{%s}}" % (interp, w, h, fname_img))
writeln(self.fh, r"\end{pgfscope}")

For one, this will use either transformshift or transformcm, and secondly, it uses the case distinction to set the image height and width directly to 1 instead of 1./f*f if there is an affine transform.
I don't really see the benefit of separating the affine transform and the translation (neither in the API nor in the output -- the user won't recognize these numbers anyway).

A doc PR to write down your hard won (sorry) understanding of how the image handling works would be appreciated!

My knowledge is still incomplete... the PDF backend has:

    def get_image_magnification(self):
        return self.image_dpi/72.0

This probably changes the units of the translation and the transform, or maybe just one of them. I can see if I can improve the base class docstrings with what I know, though.

[pwuertz]

@f0k Agreed

[f0k]

Agreed

Amended the commits to use my latest proposed code (which uses the transformshift variant you found, thanks a lot).

A doc PR to write down your hard won (sorry) understanding of how the image handling works would be appreciated!

Actually, the docs were not that bad. Part of the problem was that I had started with matplotlib 1.5.2, which had a different API and less documentation.
I added a commit to fix the documentation right here -- let me know if you'd want me to move this to a separate PR.

[pwuertz]

If you want you can also take down the #TODO note in draw_image since with your work the function is now feature complete.

[f0k]

If you want you can also take down the #TODO note in draw_image since with your work the function is now feature complete.

Did so. Wasn't sure whether the TODO referred to missing documentation of the base class or the PGF backend.

[f0k]

Explanation: It's not an "image instance", and the renderer does not have any notion of axes (as far as I can see). draw_image() is used both for axes and figure images.

[f0k]

Explanation: draw_image() does not know anything about the origin property of the AxesImage or FigureImage instance being drawn, because it doesn't get a reference to the image instance, just the image data. Hence, the meaning of y is independent of origin.

[tacaswell]

attn @mdboom

[mdboom]

This all looks correct. While we're in there, though, pixels should probably be replaced by physical units. It's pixels in Agg, and points for Pdf, Ps and Svg.

[WeatherGod]

There is still this comment that needs to be addressed, I think.

[f0k]

Explanation: This references the class to look up for further information, and it defines which units are used for the translation part of the affine transform. It also makes more clear how the two translations (x/y and transform) play together.
Also renamed trans to transform because all backends call it transform (the mismatch wasn't problematic because it's always passed as a positional argument).

[f0k]

Travis throws an error for Python 3.4, PYTHON_ARGS=-OO now:

FAIL: matplotlib.tests.test_axes.test_hist2d.test
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/matplotlib/matplotlib/venv/lib/python3.4/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/testing/decorators.py", line 55, in failer
    result = f(*args, **kwargs)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/testing/decorators.py", line 263, in do_test
    '(RMS %(rms).3f)'%err)
matplotlib.testing.exceptions.ImageComparisonFailure: images not close: /home/travis/build/matplotlib/matplotlib/result_images/test_axes/hist2d_svg.png vs. /home/travis/build/matplotlib/matplotlib/result_images/test_axes/hist2d-expected_svg.png (RMS 0.121)

Is this anything that could possibly have been caused by this PR, specifically by the last commit which changed the docstring and one argument name? Everything passed fine before. Shall we just restart Travis and hope for the best?

[tacaswell]

Please do not change kwarg name, it is a (subtle) api break for anyone who is using **dd to pass in kwargs.

[f0k]

The point is that if anybody passes trans= as a kwarg, it is already broken:

$ grep -re 'def draw_image' lib/matplotlib
lib/matplotlib/backends/backend_pgf.py:    def draw_image(self, gc, x, y, im, transform=None):
lib/matplotlib/backends/backend_template.py:    def draw_image(self, gc, x, y, im):
lib/matplotlib/backends/backend_ps.py:    def draw_image(self, gc, x, y, im, transform=None):
lib/matplotlib/backends/backend_svg.py:    def draw_image(self, gc, x, y, im, transform=None):
lib/matplotlib/backends/backend_gdk.py:    def draw_image(self, gc, x, y, im):
lib/matplotlib/backends/backend_pdf.py:    def draw_image(self, gc, x, y, im, transform=None):
lib/matplotlib/backends/backend_wx.py:    def draw_image(self, gc, x, y, im):
lib/matplotlib/backends/backend_cairo.py:    def draw_image(self, gc, x, y, im):
lib/matplotlib/backend_bases.py:    def draw_image(self, gc, x, y, im, trans=None):

The base class is the only one calling it trans, which leaves a trap for anybody using **dd to pass in the keyword arguments.

[tacaswell]

That is fun.

attn @mdboom I am inclined to accept this change and an API change entry for it.

[mdboom]

Agreed. Let's fix this to be consistent (transform everywhere) and add a note in api_changes.rst.

[f0k]

and add a note in api_changes.rst.

If I see correctly, it's already covered by https://github.com/matplotlib/matplotlib/blob/master/doc/api/api_changes/2015-12-30_draw_image.rst -- if I added another note, there would be two API changes for draw_image() listed for matplotlib 2.0. So shall we just leave it at that?

[WeatherGod]

eh, yeah there is an entry for it, but just amend it to note this particular issue, as it isn't obvious from the current item that there is this change as well.

[f0k]

Well, from the current item it isn't obvious at all what has changed:

The draw_image method implemented by backends has changed its interface.

This change is only relevant if the backend declares that it is able to transform images by returning True from option_scale_image. See the draw_image docstring for more information.

Both sentences apply to my PR as well, which just changes the changed interface. Sorry, I'm not trying to be a nuisance, but I cannot come up with a way that explicitly mentions my change without sounding silly (and confusing).

Also note that the interface change in that note belongs to #5718, which added the trans keyword in the first place, in an attempt to document the interface already present in the subclasses:
https://github.com/matplotlib/matplotlib/pull/5718/files#diff-504c288d675954e4e24de83a19242f4dR518

I'm just changing this to correctly call it transform, which has been used in subclasses even before #5718 was proposed: https://github.com/matplotlib/matplotlib/pull/5718/files#diff-2623790aa8493ba82ef737fe0e63274dL1601

So this is not actually an API change. Nobody can have used trans= as a keyword argument, because it wouldn't have worked.

Is this convincing?

[WeatherGod]

Ah, I see your point about trans. So, you are merely correcting a mistake before it reaches a release. Point taken.

[tacaswell]

While you are in here, just get rid of this test please.

[f0k]

Will do! Just let me know what to do about trans/transform, so I can amend both commits in one go.

[f0k]

Amended commits to remove this test and rebased onto latest master as I had to force-push anyway.

[f0k]

@tacaswell, @mdboom, let me know if anything else is needed. The Travis failure seems unrelated.

[mdboom]

I don't really understand the "translation given in pixels" part. The transform can include more than just translation. Maybe just say the transformation is applied in physical units?

[f0k]

The transform can include more than just translation.

Yes, but the translation is the only one that has units, everything else is unitless. I wanted to document that the translation part of the affine transform is given in pixels, not in inches.

Maybe just say the transformation is applied in physical units?

I wouldn't know what that meant. But I'd be fine with a different formulation if mine isn't clear. We should just make sure people know what to do with the transformation and the dpi factor instead of randomly trying to multiply or divide things until the demo image matches (that's about what I had to do).

Please let me know if and how to change this part!

[WeatherGod]

I am not really qualified to judge which explanation is better or more correct. @mdboom basically wrote the entire transforms system, so I have to defer to him. I do think he is right that "pixels" is the incorrect word here, and also that saying "translation" is misleading because there could be other transformations at play here.

[f0k]

I do think he is right that "pixels" is the incorrect word here

The unit used for the translation is whatever you get when you divide inches by the given dpi value. I thought this would be pixels, but I don't know the terms used in matplotlib -- let me know what you'd put here.

"translation" is misleading because there could be other transformations at play here

I see, it's ambiguous. I can write "the translation coordinates of which are given in <...>." to make clear the affine transformation does not only consist of a translation.

[tacaswell]

I think the point here is that internally mpl has 4 coordinate systems (data, axes, figure, display) and this transform needs to be a display -> display transform. In the vector backends the display units are 'points' and in the raster backends the display units are 'pixels'.

[WeatherGod]

no, you get "dots", which is the "physical unit", I think (again, this part of mpl isn't my expertise, so I could be wrong here).

As for "translation" business, I really don't know (not my area of expertise).

[f0k]

Ah, while starting to work on this, I found where I got the pixels from: From the description of parameter x, which is an additional translation given to draw_image() and uses the same units as the translation coordinates of transform. https://github.com/matplotlib/matplotlib/blame/22aa800/lib/matplotlib/backend_bases.py#L519

As you all seem to agree that "pixels" has always been the wrong word, I'll change both.

[mdboom]

For PEP8, keep lines to less than 80 characters.

[f0k]

The file already was full of lines that are too long, I thought you were ignoring E501. Should we (a) leave PEP8 to another PR, (b) change the lines I touched or (c) change all offending lines?

[WeatherGod]

Our general PEP8 policy is to 1) not introduce new PEP8 violations in new code, 2) code that is touched should get fixed, and 3) large-scale PEP8 fixes are to be kept in separate PRs.

[f0k]

Okay, modified this PR to fix long lines in my code. No PEP8 errors remaining in the parts I've touched.

[mdboom]

Looks good aside from my comments above.

[f0k]

Anything left do be done? It's good to merge from my perspective.

[WeatherGod]

Did you address @mdboom's comments? I don't see any updates to api_changes.rst.

[f0k]

Did you address @mdboom's comments?

I commented on the comment: #6792 (comment)
I don't think api_changes.rst needs an update. Let me know if I'm mistaken.

[tacaswell]

@f0k Thanks for bearing with us while we wordsmith these details.

[f0k]

I've changed all three occurrences of pixels in draw_image() (including the ones that were there since 2008) to physical units (dots or pixels), as per #6792 (comment).

physical units is not used anywhere within the matplotlib documentation, but was suggested by @mdboom in #6792 (comment). He also said the backends would either use pixels or points, which I think is wrong: A point is 1/72 of an inch, while the units here are 1/dpi of an inch.
I'm actually not sure if the translation is ever given in pixels, maybe it should always be called dots, but I'm not in a position to find out.

Thanks for bearing with us while we wordsmith these details.

Thank you. While I appreciate paying attention to details, it's certainly not very motivating to iterate that long on documenting a single method. It would have helped if somebody more familiar with the internals had taken the time to make a well-informed and consistent suggestion for the docstring. Anyway, I'm thankful matplotlib exists, is in active development and accepts contributions!

[tacaswell]

🎉 I have restarted the one failed tests (looks like one of the transient ones) and will merge as soon as that comes back green.

@f0k Thanks for digging into a subtle corner of the code base. Hopefully we will hear from you again!

[tacaswell]

backported to v2.x as e7d7a49

[f0k]

Great, thanks for merging! 👍