BUGFIX: use true bbox for rasters in backend_mixed #17182

brunobeltran · 2020-04-17T23:21:38Z

PR Summary

WIP. Needs tests. Fixes #6827, and various other analogous bugs that may or may not have issues open.

Right now, backend_mixed does something equivalent to (excuse the abuse of notation)

bbox = int(bbox/72*image_dpi)*72/image_dpi

to get the final width at which to place rastered output. The int() cast is required since rasterizing can only happen into integer numbers of pixels.

This PR adds some methods to MixedModeRenderer that wrap each draw_* function, allowing us to manually keep around the original desired bbox of each rasterized component, allowing it to be scaled back to fit correctly.

This means that for mixed-mode renders, dpi= is now a suggestion (the true dpi will typically be off by <1%, but can be off by quite a bit for very low dpi requests). However, most people using vector output care less about the exact dpi and care a lot about things not looking randomly out of alignment.

This approach was suggested by @jklymak in the discussion of #16488.

The tests will stop failing once I've reimplented all of

    * :meth:`draw_path`
    * :meth:`draw_image`
    * :meth:`draw_gouraud_triangle`
    * :meth:`draw_text`
    * :meth:`draw_markers`
    * :meth:`draw_path_collection`
    * :meth:`draw_quad_mesh`

for MixedModeRenderer.

Comparison with previous behavior

Code

import matplotlib as mpl
import matplotlib.pyplot as plt
import matplotlib.cm
import numpy as np

mpl.rcParams['figure.dpi'] = 20
fig, ax = plt.subplots(figsize=(2, 1.5))

ax.imshow(np.array([[0, 1], [1, 0]]))
ax.axis('off')
sm = matplotlib.cm.ScalarMappable()
sm.set_array([])
plt.colorbar(sm)

plt.savefig('test.svg')

Old output

Bboxes added in post, for emphasis:

New output

Bboxes align exactly

PR Checklist

Has Pytest style unit tests
Code is Flake 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

jklymak · 2020-04-17T23:32:24Z

This looks far better!

I don't see why anyone would care if their raster in a vector graphic was 300.5 dpi instead of 300, but perhaps we should make sure there are no practical reasons that is a problem. @jkseppan is the PDF backend expert and may have some thoughts.

So do those methods all really need to be re-implimented? Before you do all that can we check that you are doing this the easiest way?

I took a quick look at the code this morning, and it does seem something will have to bee added because draw_image loses track of how exactly how wide the image is meant to be once it figures out how many "pixels" wide it will be. That makes perfect sense for actual raster outputs, but is not applicable here.

brunobeltran · 2020-04-18T01:33:58Z

Thanks for helping out Jody!

So do those methods all really need to be re-implemented? Before you do all that can we check that you are doing this the easiest way?

The methods don't need to be reimplemented, we just need to write small little wrappers that compute the bbox in each case, but would also be happy to hear if there is a cleaner way!

It's unfortunate, because at the level of the individual Artist.draw calls, the artist always has a get_clipbox that could be read directly to grab the "real" bbox. But the first call that touches the renderer is a renderer.draw_* call, and none of the parameters of these methods have information about the size of the object (so I have to recalculate it in my wrappers.

For example the colorbar is a QuadMesh, so even though we know its absolute size inside of QuadMesh.draw (via e.g. QuadMesh.get_clip_box), once QuadMesh.draw calls renderer.draw_quad_mesh, (which I wrap), I have to recompute the Bbox by hand, which is very wasteful.

anntzer · 2020-04-18T08:30:49Z

lib/matplotlib/image.py

@@ -144,7 +144,11 @@ def flush_images():
                    gc = renderer.new_gc()
                    gc.set_clip_rectangle(parent.bbox)
                    gc.set_clip_path(parent.get_clip_path())
-                    renderer.draw_image(gc, round(l), round(b), data)
+                    if type(renderer) == mpl.backends.backend_agg.RendererAgg:


Hadn't thought of third-party backends. With your suggestion, it would be something like...

if hasattr(renderer, 'draw_image_2'):

but this would effectively be deprecating a pretty core rendering function....so probably not a good idea?

anntzer · 2020-04-18T08:36:23Z

lib/matplotlib/backends/backend_cairo.py

@@ -217,7 +217,7 @@ def draw_markers(self, gc, marker_path, marker_trans, path, transform,
            self._fill_and_stroke(
                ctx, rgbFace, gc.get_alpha(), gc.get_forced_alpha())

-    def draw_image(self, gc, x, y, im):
+    def draw_image(self, gc, x, y, im, bbox=None):


What is supposed to happen if the bbox corner is not consistent with the (x, y)? Should this really rather be a new method draw_image_2(self, gc, bbox, im): ...? This will make it easier to check whether third-party backends implement that method, too. (Or if we keep the same name, we may need signature-sniffing tricks to figure out signature to use... or something like option_scale_image.)

Didn't think of third-party backends. I guess I could implement it as a separate method.

Actually, to prevent having to do some weird deprecation stuff to draw_image, I think the better solution is to just add optional width and height arguments instead, and yeah....a hasattr trick to check what signature to use would work....even though it feels icky to write. Like option_exact_image_bbox.

brunobeltran · 2020-04-18T09:18:27Z

lib/matplotlib/image.py

+                        renderer.draw_image(gc, l, b, im)
+                    else:
+                        renderer.draw_image(gc, round(l), round(b), data,
+                                            bbox=parent.bbox)


Turns out this is wrong (the test failure test_image.py:test_bbox_image_inverted is real), since BboxImages are allowed to fall outside the axes (parent) bbox.

There are two ways that draw_image gets called. By stop_rasterizing and by (_ImageBase).draw (here). The latter is correctly dealt with in this PR already, but the former needs a more "correct" fix than passing the parent bbox.

anntzer · 2020-04-18T09:31:01Z

Do you really need to pass in the true bbox explicitly? Can the vector backends not infer it by applying transform to (x, y, im.width, im.height)? Possibly the caller may need to adjust transform a bit, though. But then no need for signature changes.

brunobeltran · 2020-04-18T10:14:14Z

Do you really need to pass in the true bbox explicitly? Can the vector backends not infer it by applying transform to (x, y, im.width, im.height)? Possibly the caller may need to adjust transform a bit, though. But then no need for signature changes.

As far as I can tell, the information about what the original size of the figure is has been lost by the time that you reach the vector backend. We could in principle hijack the transform kwarg to get that information in there....it's not hard to construct the correct translation+scaling (as in the current PR's version of backend_svg.draw_image).

I'm happy with whatever everyone agrees is the best/minimal change to the interface.

However, the bigger issue is that it looks like I have to make an API change of some sort anyway in order to have the actual extents available to pass to draw_image in the first place.

There are two-ish places where draw_image is called (in image.py:_ImageBase.draw and backend_mixed.py:MixedModeRenderer.stop_rasterizing). This PR already correctly deals with the latter case, but the former case actually not quite.

What I currently do (use the Bbox of the parent passed to _draw_list_compositing_images) works for FigureImage and AxesImage objects because in those cases parent is the Figure/Axes). However, for BboxImage's, that is not necessarily the correct bounding box.

Solutions I can think of:

Bump get_window_extent to the superclass _ImageBase, so that we can call it in _ImageBase.draw to get the correct pre-rounding bbox to pass to the vectorized renderers

Happy to hear if people approve and/or have better ideas?

brunobeltran · 2020-04-18T10:16:53Z

That said, maybe in terms of scope, we could leave image.py as-is in this PR, and only modify the draw_image call in MixedModeRenderer.stop_rasterizing. Then, any _BaseImage subclass will still be drawn not aligned to its edge, but at least we'll have fixed the bug for every other rasterized object that MixedModeRenderer deals with (color bars in particular)?

anntzer · 2020-04-18T10:54:50Z

May be easier to discuss this over a call (at least right now I have trouble keeping track of all moving parts); let's try to discuss this on Monday or possibly separately?

anntzer · 2020-04-19T10:08:16Z

Having a look at it again briefly (sorry for doing this in an on-and-off manner), I think you can indeed just smuggle the relevant information in the transform kwarg (and hence have no external API change) by doing the relevant work in make_image/_make_image?

brunobeltran · 2020-04-20T01:34:34Z

Not at all, thanks for looking into it! You're right, I'll push up a version with no external API change before the meeting tomorrow :)

brunobeltran · 2020-04-20T14:10:58Z

we could leave image.py as-is in this PR, and only modify the draw_image call in MixedModeRenderer.stop_rasterizing

Decided that in order to keep this PR sane, I have to restrict it to only fix half of the "bug".

The new version I just pushed fixes the colorbar bug, but leaves the imshow bug as-is. Separate PR for imshow bug incoming.

My new fix seems to work great, unless someone requests a rasterized Path during a call to a MixedModeRenderer and the linewidth is larger than a few pixels (no idea why you would do such a thing, since the whole idea behind mixed mode renderers is to be able to have paths vectorized).

This corner case has a regression (ironically, in tests/test_image.py:rasterize_10dpi_pdf).

The lines in the middle and on the right are supposed to look the same:

The issue is that my new code uses the following trick:

keep track of the bbox of each rasterized area by wrapping calls to draw_marker, draw_path, draw_quad_mesh, etc. in MixedModeRenderer.
at the end of each rasterization, scale the rendered pixels to the final "theoretical" bbox we've calculated.

That works great, except for in order to calculate the correct bbox for a stroked path, we need to wait for the code in #17198. Right now, although we merged some fixes to path extents calculations, we still assume a "zero" thickness path when computing its extents.

#17198 is a big PR. But until it gets merged, we need to accept this regression (the users that would trigger such a thing, I'm not very sympathetic to, personally, whereas I do think we should be able to render colorbars without them being super ugly) or come up with a whole new strategy, that doesn't involve pre-computing where each rasterization will be placed in the final vector image.

The other baseline_images changes are "more" correct than they used to be.

jklymak · 2020-09-21T17:19:56Z

@brunobeltran where is this PR at?

jklymak · 2021-05-09T19:34:27Z

ping @brunobeltran This will fix a lot of long-standing bugs. If you have abandoned it, please let us know and someone will pick it up...

jklymak · 2021-06-14T18:55:02Z

I'll ping again, and marking as stale...

jklymak · 2021-10-02T07:51:39Z

I've added to the 3.6 milestone and to the project board as I think this is quite high priority. Its a little ridiculous that our vector graphic rasters snap to virtual pixels.

jklymak · 2021-11-19T07:27:57Z

ping @brunobeltran - I'll take this over if you can't get back to this as its pretty high priority. We get bug reports every couple of months about this...

tacaswell · 2022-12-16T00:28:22Z

pro: I think I got the rebase to work!
con: there are a bunch of test failures I do not understand locally

tacaswell · 2022-12-16T00:54:19Z

ok understand one of the errors:

this PR does not wrap draw_markers yet

tacaswell · 2022-12-16T00:55:06Z

Unfortunately I think this is going to have to get punted a version again.

Co-authored-by: Jody Klymak <jklymak@gmail.com> Co-authored-by: Thomas A Caswell <tcaswell@gmail.com>

jklymak · 2023-04-23T16:48:40Z

This version passes all tests, and adds one more test to ps backend. Obviously some image changes.

It does not wrap markers so that is still a todo, but it doesn't make the situation worse.

@anntzer suggest wrapping this in the transform object. I think thats true, but seems more confusing than handling this directly.

anntzer · 2023-04-23T21:40:56Z

@anntzer suggest wrapping this in the transform object. I think thats true, but seems more confusing than handling this directly.

I only looked at this briefly again. Looks like since my last comment (and probably since #17182 (comment)) the API changes are now limited to the addition of the true_size kwarg which effectively(?) serves as a private communication channel between MixedModeRenderer and the builtin vector backends, which is not so much of an issue (at least on mplcairo's side I just completely skip trying to work with MixedModeRenderer...).

Still it's not entirely clear to me what the semantics of draw_image(gc, x, y, im, transform, true_size) now really means. Previously draw_image(gc, x, y, im, transform) meant "draw the image at position x, y (in pixels); the image pixels (0..m, 0..n) should be transformed by transform first and that also indicates the size of the rendered image". How does that now interact with true_size? Perhaps that's simple enough, but should be documented.

Minor review points: true_size should probably be true_shape (for consistency with ndarray.shape/ndarray.size), and option_true_bbox_image is no longer needed.

jklymak · 2023-04-23T23:18:28Z

Still it's not entirely clear to me what the semantics of draw_image(gc, x, y, im, transform, true_size) now really means.

I think that is unchanged. The new codepaths are only if transform is None.

oscargus · 2023-04-24T08:23:30Z

lib/matplotlib/backends/backend_mixed.py

+                gc, master_transform, meshWidth, meshHeight, coordinates,
+                offsets, offsetTrans, facecolors, antialiased, edgecolors)
+
+    def draw_gouraud_triangle(self, gc, points, colors, transform):


This should probably be draw_gouraud_triangles:

matplotlib/lib/matplotlib/backend_bases.py

Lines 291 to 292 in d77801e

@_api.deprecated("3.7", alternative="draw_gouraud_triangles")

def draw_gouraud_triangle(self, gc, points, colors, transform):

https://matplotlib.org/stable/api/prev_api_changes/api_changes_3.7.0.html#draw-gouraud-triangle

oscargus · 2023-04-24T08:25:19Z

lib/matplotlib/tests/baseline_images/test_image/rasterize_10dpi.svg

This seems to be a significant change. Is this correct?

No it's incorrect. Thanks for catching!

This is actually a pretty big problem - the "True" bbox is calculated from the path, but the image bbox includes the full stroke. The the "True" bbox should really be calculated from the path+stroke.

My tendency is to just not do the fine tuning for paths - I expect they are rarely worth rasterizing anyhow.

anntzer · 2023-04-24T08:44:36Z

I think that is unchanged. The new codepaths are only if transform is None.

Fair enough, but this should then be documented.

jklymak requested a review from anntzer April 17, 2020 23:26

jklymak marked this pull request as draft April 17, 2020 23:32

jklymak added status: needs comment/discussion needs consensus on next step backend: pdf backend: ps backend: svg labels Apr 17, 2020

jklymak added this to the v3.4.0 milestone Apr 17, 2020

anntzer reviewed Apr 18, 2020

View reviewed changes

brunobeltran commented Apr 18, 2020

View reviewed changes

brunobeltran changed the title ~~BUGFIX: use true bbox for rasters in backend_mixed~~ Fix mixed-mode rasters to align with intended bounding boxes in vectorized output Apr 19, 2020

brunobeltran changed the title ~~Fix mixed-mode rasters to align with intended bounding boxes in vectorized output~~ BUGFIX: use true bbox for rasters in backend_mixed Apr 19, 2020

brunobeltran mentioned this pull request Apr 20, 2020

Stroked path width #17198

Draft

6 tasks

brunobeltran force-pushed the colorbar-rounding-fix branch from 1b0a9d4 to b99dec2 Compare April 20, 2020 13:52

brunobeltran force-pushed the colorbar-rounding-fix branch from b99dec2 to 66b0a11 Compare April 20, 2020 15:03

QuLogic added the status: needs tests label Sep 22, 2020

QuLogic modified the milestones: v3.4.0, v3.5.0 Jan 21, 2021

jklymak mentioned this pull request Nov 18, 2021

[Bug]: 3.5.0 colorbar outside of its axis when saved into vectorial formats #21672

Closed

Spl1x mentioned this pull request Feb 23, 2022

[Bug]: Rounding to pixel boundary misaligns images in vector graphics outputs #20964

Open

QuLogic modified the milestones: v3.6.0, v3.7.0 Jul 5, 2022

jklymak mentioned this pull request Jul 6, 2022

[Bug]: Irregular pixel widths in rasterized pcolor #23391

Closed

github-actions bot added the status: needs rebase label Oct 18, 2022

tacaswell force-pushed the colorbar-rounding-fix branch from 66b0a11 to 5fb7665 Compare December 16, 2022 00:27

github-actions bot removed the status: needs rebase label Dec 16, 2022

tacaswell modified the milestones: v3.7.0, v3.8.0 Dec 16, 2022

jklymak mentioned this pull request Apr 16, 2023

Incorrect imshow extent in PDF backend #8981

Closed

jklymak force-pushed the colorbar-rounding-fix branch from 36cbda3 to 1869fc3 Compare April 22, 2023 22:54

BF: MixedModeRenderer scale rasters to "true" bbox

bd3ff10

Co-authored-by: Jody Klymak <jklymak@gmail.com> Co-authored-by: Thomas A Caswell <tcaswell@gmail.com>

jklymak force-pushed the colorbar-rounding-fix branch from 3a091c7 to bd3ff10 Compare April 23, 2023 15:27

jklymak marked this pull request as ready for review April 23, 2023 16:48

oscargus reviewed Apr 24, 2023

View reviewed changes

jklymak marked this pull request as draft April 24, 2023 15:12

github-actions bot added the status: needs rebase label Jun 13, 2023

ksunden modified the milestones: v3.8.0, future releases Aug 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUGFIX: use true bbox for rasters in backend_mixed #17182

BUGFIX: use true bbox for rasters in backend_mixed #17182

brunobeltran commented Apr 17, 2020

jklymak commented Apr 17, 2020

brunobeltran commented Apr 18, 2020

anntzer Apr 18, 2020

brunobeltran Apr 18, 2020

anntzer Apr 18, 2020

brunobeltran Apr 18, 2020 •

edited

Loading

brunobeltran Apr 18, 2020

brunobeltran Apr 18, 2020

anntzer commented Apr 18, 2020

brunobeltran commented Apr 18, 2020

brunobeltran commented Apr 18, 2020

anntzer commented Apr 18, 2020

anntzer commented Apr 19, 2020

brunobeltran commented Apr 20, 2020

brunobeltran commented Apr 20, 2020 •

edited

Loading

jklymak commented Sep 21, 2020

jklymak commented May 9, 2021

jklymak commented Jun 14, 2021

jklymak commented Oct 2, 2021

jklymak commented Nov 19, 2021

tacaswell commented Dec 16, 2022

tacaswell commented Dec 16, 2022

tacaswell commented Dec 16, 2022

jklymak commented Apr 23, 2023

anntzer commented Apr 23, 2023

jklymak commented Apr 23, 2023

oscargus Apr 24, 2023

oscargus Apr 24, 2023

jklymak Apr 24, 2023

jklymak Apr 24, 2023

anntzer commented Apr 24, 2023

	@_api.deprecated("3.7", alternative="draw_gouraud_triangles")
	def draw_gouraud_triangle(self, gc, points, colors, transform):

BUGFIX: use true bbox for rasters in backend_mixed #17182

Are you sure you want to change the base?

BUGFIX: use true bbox for rasters in backend_mixed #17182

Conversation

brunobeltran commented Apr 17, 2020

PR Summary

Comparison with previous behavior

Code

Old output

New output

PR Checklist

jklymak commented Apr 17, 2020

brunobeltran commented Apr 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brunobeltran Apr 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anntzer commented Apr 18, 2020

brunobeltran commented Apr 18, 2020

brunobeltran commented Apr 18, 2020

anntzer commented Apr 18, 2020

anntzer commented Apr 19, 2020

brunobeltran commented Apr 20, 2020

brunobeltran commented Apr 20, 2020 • edited Loading

jklymak commented Sep 21, 2020

jklymak commented May 9, 2021

jklymak commented Jun 14, 2021

jklymak commented Oct 2, 2021

jklymak commented Nov 19, 2021

tacaswell commented Dec 16, 2022

tacaswell commented Dec 16, 2022

tacaswell commented Dec 16, 2022

jklymak commented Apr 23, 2023

anntzer commented Apr 23, 2023

jklymak commented Apr 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anntzer commented Apr 24, 2023

brunobeltran Apr 18, 2020 •

edited

Loading

brunobeltran commented Apr 20, 2020 •

edited

Loading