Artist's draw method prevents rasterization by default #24484

leejjoon · 2022-11-17T13:06:47Z

PR Summary

This is another PR to address #23999
(An alternative PR was proposed #24473 but was not favored as it prevents merging rasterized artists together)

It utilizes Artist.__init_subclass__ to decorate its draw method by defaullt with _prevent_rasterization, so that the rasterization can be properly stopped when an artist that does not support rasterization is encountered.

Documentation and Tests

Has pytest style unit tests (and pytest passes)
[N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).
[N/A] New plotting related features are documented with examples.

Release Notes

[N/A] New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
[N/A] API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

lib/matplotlib/artist.py

lib/matplotlib/tests/test_backend_svg.py

tacaswell · 2022-11-17T14:28:48Z

lib/matplotlib/artist.py

+    # (draw._supports_rasterization is True), it won't  be decorated by
+    # `_prevent_rasterization`.
+
+    if hasattr(draw, "_supports_rasterization"):


Do we also want to set draw._supports_rasterization = False in this wrapper?

I have no preference, as far as we use the current condition of getattr(self.draw, "_supports_rasterization", False) .
And not sure which is better.

On one hand, not setting it is the safer minimal change, but setting it to False (and finding + fixing our internal references to use getattr(obj, '_supports_rasterization', False) instead) makes everything more consistent and understandable for the next set of people.

I will not block merging this over sorting that out.

The only occurrence of _supports_rasterization is in the Artist.set_rasterized method (which already does getattr(obj, '_supports_rasterization', False). And I will go ahead and add draw._supports_rasterization = False here.

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

tacaswell · 2022-11-17T14:43:01Z

👍🏻 I think this is on the right path.

QuLogic · 2022-11-18T23:29:30Z

lib/matplotlib/artist.py

@@ -106,6 +131,8 @@ def __init_subclass__(cls):
        # Inject custom set() methods into the subclass with signature and
        # docstring based on the subclasses' properties.

+        cls.draw = _prevent_rasterization(cls.draw)


Either the above comment needs updating, or this should not go between it and the code below.

Thanks. I will move this line above the comment.

We should be more careful here about not not over-writing the draw method unless we have to, I will push a commit to try and fix this.

lib/matplotlib/artist.py

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>

leejjoon · 2022-11-21T07:25:31Z

Somehow, ci/circleci:docs-python38 fails, and I could not able to figure out why. Can someone take a look into this?
Once this issue is resolved, I may squash the commits into a single one.

ksunden · 2022-11-21T18:38:24Z

Doc failure is because of these warnings:

https://app.circleci.com/pipelines/github/matplotlib/matplotlib/20292/workflows/2bc0d7fe-b1f7-42d0-9045-50662e2fe89e/jobs/72240?invite=true#step-113-1162

The links in some of the old deprecation/removal files are underspecified, as there are multiple options that it could choose.

I think that before this change, the Axes.draw method was not overridden for these subclasses so sphinx was happy to just select the base class when asked for Axes.draw.

With this change, the subclasses still get a setattr done to draw, and thus sphinx actually generates a method description that it did not previously.

(Note that current docs for axislines.Axes do not render a draw method as it is just provided by the parent class, but this PR does )

This can be resolved to make sphinx happy by being more explicit about wanting matplotlib.axes.Axes.draw in the old deprecation/removal RST files.

tacaswell · 2022-11-23T23:58:55Z

@leejjoon I took the liberty of pushing an extra commit to fix a subtle issue with (deep) class hierarchies. I hope you do not mind.

leejjoon · 2022-11-24T00:22:09Z

@leejjoon I took the liberty of pushing an extra commit to fix a subtle issue with (deep) class hierarchies. I hope you do not mind.

No, not at all.

However, the same check is also done inside the decorator,

matplotlib/lib/matplotlib/artist.py

Line 33 in 9de19d0

if hasattr(draw, "_supports_rasterization"):

which I think is now redundant. Do you want it to be removed?

-JJ

ksunden · 2022-11-24T02:09:00Z

Putting the check at this level has some subtle distinctions for subclasses and behavior of super calls, and in fact removes the conflicting entries from the doc builds (though the changes to address those are not worth undoing)

leejjoon · 2022-11-26T12:13:47Z

Putting the check at this level has some subtle distinctions for subclasses and behavior of super calls, and in fact removes the conflicting entries from the doc builds (though the changes to address those are not worth undoing)

Thanks. I did not know if that will make a difference. I have removed the if statement inside the decorator.

tacaswell · 2022-12-09T20:16:03Z

Thank you @leejjoon !

tacaswell · 2022-12-09T20:16:14Z

Thank you @leejjoon !

dstansby · 2022-12-13T18:38:29Z

👋 unfortunately this has broken some plotting in astropy. In astropy.visualization.wcsaxes we have a situation like:

from matplotlib.text import Text
import matplotlib.pyplot as plt
from matplotlib.artist import Artist

class CustomArtist(Artist):
    def draw(self, renderer):
        t = CustomText(0, 0, 'a')
        t.draw(renderer, custom_param='custom_param_value')

class CustomText(Text):
    def draw(self, renderer, custom_param=None):
        super().draw(renderer)


fig, ax = plt.subplots()
ax.add_artist(CustomArtist())
plt.show()

The key here is the dummy custom_param (in astropy this parameter is used). Before this PR everything was fine (my sample code above fails, but on a different error), but after this PR the error below is raised. I think astropy is doing Bad Things by having parameters in the overriden draw() methods, and I am working on changing it (astropy/astropy#12672) but making slow progress.

Would be very greatful if there's an easy way to fix this for astropy. If not, and we need to fix on our end by removing extra arguments to draw(), please let me know and I can use that as motivation to get that change done quicker!

Traceback (most recent call last):
  File "/Users/dstansby/github/matplotlib/lib/matplotlib/backend_bases.py", line 1212, in _on_timer
    ret = func(*args, **kwargs)
          ^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dstansby/github/matplotlib/lib/matplotlib/backends/backend_macosx.py", line 68, in callback_func
    callback()
  File "/Users/dstansby/github/matplotlib/lib/matplotlib/backends/backend_macosx.py", line 88, in _draw_idle
    self.draw()
  File "/Users/dstansby/github/matplotlib/lib/matplotlib/backends/backend_macosx.py", line 50, in draw
    super().draw()
  File "/Users/dstansby/github/matplotlib/lib/matplotlib/backends/backend_agg.py", line 400, in draw
    self.figure.draw(self.renderer)
  File "/Users/dstansby/github/matplotlib/lib/matplotlib/artist.py", line 95, in draw_wrapper
    result = draw(artist, renderer, *args, **kwargs)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dstansby/github/matplotlib/lib/matplotlib/artist.py", line 72, in draw_wrapper
    return draw(artist, renderer)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dstansby/github/matplotlib/lib/matplotlib/figure.py", line 3077, in draw
    mimage._draw_list_compositing_images(
  File "/Users/dstansby/github/matplotlib/lib/matplotlib/image.py", line 131, in _draw_list_compositing_images
    a.draw(renderer)
  File "/Users/dstansby/github/matplotlib/lib/matplotlib/artist.py", line 72, in draw_wrapper
    return draw(artist, renderer)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dstansby/github/matplotlib/lib/matplotlib/axes/_base.py", line 3148, in draw
    mimage._draw_list_compositing_images(
  File "/Users/dstansby/github/matplotlib/lib/matplotlib/image.py", line 131, in _draw_list_compositing_images
    a.draw(renderer)
  File "/Users/dstansby/github/matplotlib/lib/matplotlib/artist.py", line 39, in draw_wrapper
    return draw(artist, renderer)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dstansby/github/matplotlib/test.py", line 8, in draw
    t.draw(renderer, custom_param='custom_param_value')
TypeError: CustomText.draw() got an unexpected keyword argument 'custom_param'

tacaswell · 2022-12-13T22:26:44Z

The wrapper should blindly forward *args, **kwargs, sorry for not thinking of this case on review.

tacaswell · 2022-12-13T22:46:32Z

diff --git a/lib/matplotlib/artist.py b/lib/matplotlib/artist.py
index a4693f78cf..7967f597d4 100644
--- a/lib/matplotlib/artist.py
+++ b/lib/matplotlib/artist.py
@@ -29,14 +29,14 @@ def _prevent_rasterization(draw):
     # (e.g., change in dpi).
 
     @wraps(draw)
-    def draw_wrapper(artist, renderer):
+    def draw_wrapper(artist, renderer, *args, **kwargs):
         if renderer._raster_depth == 0 and renderer._rasterizing:
             # Only stop when we are not in a rasterized parent
             # and something has been rasterized since last stop.
             renderer.stop_rasterizing()
             renderer._rasterizing = False
 
-        return draw(artist, renderer)
+        return draw(artist, renderer, *args, **kwargs)
 
     draw_wrapper._supports_rasterization = False
     return draw_wrapper

should be enough, working on a PR.

tacaswell · 2022-12-13T22:57:26Z

from matplotlib.text import Text
import matplotlib.pyplot as plt
from matplotlib.artist import Artist

class CustomArtist(Artist):
    def draw(self, renderer):
        t = CustomText(0, 0, 'a', figure=self.figure)
        t.draw(renderer, custom_param='custom_param_value')

class CustomText(Text):
    def draw(self, renderer, custom_param=None):
        super().draw(renderer)


fig, ax = plt.subplots()
ax.add_artist(CustomArtist())
plt.show()

Fixes the other issue.

leejjoon added 2 commits November 17, 2022 21:17

Artist's draw method by default prevents rasterization

e83ff55

add a test case of test_prevent_rasterization

fa58bde

leejjoon changed the title ~~Artist's draw method prevent rasterization by default~~ Artist's draw method prevents rasterization by default Nov 17, 2022

tacaswell added this to the v3.7.0 milestone Nov 17, 2022

tacaswell reviewed Nov 17, 2022

View reviewed changes

lib/matplotlib/artist.py Outdated Show resolved Hide resolved

tacaswell reviewed Nov 17, 2022

View reviewed changes

lib/matplotlib/artist.py Outdated Show resolved Hide resolved

tacaswell reviewed Nov 17, 2022

View reviewed changes

lib/matplotlib/tests/test_backend_svg.py Outdated Show resolved Hide resolved

tacaswell reviewed Nov 17, 2022

View reviewed changes

leejjoon and others added 2 commits November 17, 2022 23:38

Update lib/matplotlib/artist.py

7bf0c52

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

Update lib/matplotlib/tests/test_backend_svg.py

34aa28e

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

leejjoon added 2 commits November 17, 2022 23:45

update artist.py

da3abc8

more minor fix

ab776e5

tacaswell approved these changes Nov 18, 2022

View reviewed changes

tacaswell mentioned this pull request Nov 18, 2022

Calls stop_rasterization early #24473

Closed

7 tasks

QuLogic reviewed Nov 18, 2022

View reviewed changes

leejjoon and others added 5 commits November 20, 2022 11:34

Update lib/matplotlib/artist.py

20620ef

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>

Update lib/matplotlib/artist.py

6070ba6

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>

Update lib/matplotlib/artist.py

d35b911

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>

Update lib/matplotlib/artist.py

91814e8

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>

minor fix

61e9da9

leejjoon and others added 2 commits November 22, 2022 09:27

fix warning in doc build

b358e6d

MNT: be more gentle about resetting the draw method

9de19d0

remove redundant if statement.

d8d2c68

ksunden approved these changes Dec 9, 2022

View reviewed changes

tacaswell merged commit cca039d into matplotlib:main Dec 9, 2022

dstansby mentioned this pull request Dec 13, 2022

draw_wcsaxes broken on matplotlib dev astropy/astropy#14174

Closed

QuLogic mentioned this pull request Jan 25, 2023

[Bug]: Annotation misplaced when rasterizing and saving as PDF #23999

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Artist's draw method prevents rasterization by default #24484

Artist's draw method prevents rasterization by default #24484

leejjoon commented Nov 17, 2022

tacaswell Nov 17, 2022

leejjoon Nov 17, 2022

tacaswell Nov 17, 2022

tacaswell Nov 17, 2022

leejjoon Nov 20, 2022

tacaswell commented Nov 17, 2022

QuLogic Nov 18, 2022

leejjoon Nov 20, 2022

tacaswell Nov 23, 2022

leejjoon commented Nov 21, 2022

ksunden commented Nov 21, 2022

tacaswell commented Nov 23, 2022

leejjoon commented Nov 24, 2022

ksunden commented Nov 24, 2022

leejjoon commented Nov 26, 2022

tacaswell commented Dec 9, 2022

tacaswell commented Dec 9, 2022

dstansby commented Dec 13, 2022

tacaswell commented Dec 13, 2022

tacaswell commented Dec 13, 2022

tacaswell commented Dec 13, 2022

Artist's draw method prevents rasterization by default #24484

Artist's draw method prevents rasterization by default #24484

Conversation

leejjoon commented Nov 17, 2022

PR Summary

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

Choose a reason for hiding this comment

tacaswell commented Nov 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leejjoon commented Nov 21, 2022

ksunden commented Nov 21, 2022

tacaswell commented Nov 23, 2022

leejjoon commented Nov 24, 2022

ksunden commented Nov 24, 2022

leejjoon commented Nov 26, 2022

tacaswell commented Dec 9, 2022

tacaswell commented Dec 9, 2022

dstansby commented Dec 13, 2022

tacaswell commented Dec 13, 2022

tacaswell commented Dec 13, 2022

tacaswell commented Dec 13, 2022