Skip to content

Undesirable behaviour of MixedModeRenderer #13021

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Mar 18, 2019
Merged

Undesirable behaviour of MixedModeRenderer #13021

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b85c63b
FIX: Correctly proxy the renderers in MixedModeRenderer.
pelson Dec 17, 2018
d8775ae
Added a test and baseline images. Unable to test SVG due to lack of i…
pelson Dec 19, 2018
0676c17
Remove the untested SVG.
pelson Dec 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 12 additions & 23 deletions lib/matplotlib/backends/backend_mixed.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,27 +62,16 @@ def __init__(self, figure, width, height, dpi, vector_renderer,

self._bbox_inches_restore = bbox_inches_restore

self._set_current_renderer(vector_renderer)

_methods = """
close_group draw_image draw_markers draw_path
draw_path_collection draw_quad_mesh draw_tex draw_text
finalize flipy get_canvas_width_height get_image_magnification
get_texmanager get_text_width_height_descent new_gc open_group
option_image_nocomposite points_to_pixels strip_math
start_filter stop_filter draw_gouraud_triangle
draw_gouraud_triangles option_scale_image
_text2path _get_text_path_transform height width
""".split()

def _set_current_renderer(self, renderer):
self._renderer = renderer

for method in self._methods:
if hasattr(renderer, method):
setattr(self, method, getattr(renderer, method))
renderer.start_rasterizing = self.start_rasterizing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was stuffing methods from the proxy renderer on to the proxied renderer. Is it a problem that we are going to lose this?

renderer.stop_rasterizing = self.stop_rasterizing
self._renderer = vector_renderer

def __getattr__(self, attr):
# Proxy everything that hasn't been overridden to the base
# renderer. Things that *are* overridden can call methods
# on self._renderer directly, but must not cache/store
# methods (because things like RendererAgg change their
# methods on the fly in order to optimise proxying down
# to the underlying C implementation).
return getattr(self._renderer, attr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer explicitly listing the functions that get forwarded to the renderer, whether by checking attr in _methods in __getattr__, or by writing a bunch of forwarding functions

for _attr in _methods:
    locals()[_attr] = lambda self, *args, **kwargs: getattr(self._renderer, _attr)(*args, **kwargs)

(in spirit; add a call to functools.wraps, etc.)

The reason is that the Renderer API is already quite large, and while implementing https://github.com/anntzer/mplcairo I found out that there are some methods that are not documented but are actually needed for a fully functional renderer backend; making the list explicit should hopefully avoid letting that list further grow out of control.

Copy link
Member Author

@pelson pelson Dec 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see your point of view here @anntzer, but I disagree. In this case we are implementing a renderer proxy. A proxy, IMO, should forward everything on to the thing it is proxying. It is the thing it's proxying's responsibility to define what is and isn't allowed.

In that regard, the list of methods should be documented in the base, not the proxy. Indeed, they are partially documented at https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/backend_bases.py#L135-L145, with the remainder being only documented as stub methods.

The simplest argument against explicitly naming all methods is this:

if we update the base renderer to have new capabilities, we should not have to update the proxy renderer

But also:

if a subclass of a base renderer implements it own methods, the proxy renderer should honour (i.e. proxy) them

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with not explicitly listing the methods here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Away from serious reviewing for the Christmas period but the complete proxying approach of this PR is fine too even though I think (per the comment below) that a more explicit approach is better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also agree that we should proxy out everything.


def start_rasterizing(self):
"""
Expand All @@ -105,7 +94,7 @@ def start_rasterizing(self):
if self._rasterizing == 0:
self._raster_renderer = self._raster_renderer_class(
self._width*self.dpi, self._height*self.dpi, self.dpi)
self._set_current_renderer(self._raster_renderer)
self._renderer = self._raster_renderer
self._rasterizing += 1

def stop_rasterizing(self):
Expand All @@ -119,7 +108,7 @@ def stop_rasterizing(self):
"""
self._rasterizing -= 1
if self._rasterizing == 0:
self._set_current_renderer(self._vector_renderer)
self._renderer = self._vector_renderer

height = self._height * self.dpi
buffer, bounds = self._raster_renderer.tostring_rgba_minimized()
Expand Down
Binary file added lib/matplotlib/tests/baseline_images/test_agg_filter/agg_filter_alpha.pdf
View file
Open in desktop
Binary file not shown.
Binary file added lib/matplotlib/tests/baseline_images/test_agg_filter/agg_filter_alpha.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
29 changes: 29 additions & 0 deletions lib/matplotlib/tests/test_agg_filter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import numpy as np

import matplotlib.pyplot as plt
from matplotlib.testing.decorators import image_comparison


@image_comparison(baseline_images=['agg_filter_alpha'])
def test_agg_filter_alpha():
ax = plt.axes()
x, y = np.mgrid[0:7, 0:8]
data = x**2 - y**2
mesh = ax.pcolormesh(data, cmap='Reds', zorder=5)

def manual_alpha(im, dpi):
im[:, :, 3] *= 0.6
print('CALLED')
return im, 0, 0

# Note: Doing alpha like this is not the same as setting alpha on
# the mesh itself. Currently meshes are drawn as independent patches,
# and we see fine borders around the blocks of color. See the SO
# question for an example: https://stackoverflow.com/questions/20678817
mesh.set_agg_filter(manual_alpha)

# Currently we must enable rasterization for this to have an effect in
# the PDF backend.
mesh.set_rasterized(True)

ax.plot([0, 4, 7], [1, 3, 8])