Add fig.add_artist method #11234

ImportanceOfBeingErnest · 2018-05-11T22:40:05Z

PR Summary

As quickly discussed on gitter, an add_artist() of Figure may be useful in some cases.
This PR would add such method.

PR Checklist

Has Pytest style unit tests
Code is PEP 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)

jklymak

Looks great asside frm the small formatting issues! Thanks!

jklymak · 2018-05-11T23:10:07Z

lib/matplotlib/figure.py

+        Add any :class:`~matplotlib.artist.Artist` to the figure.
+
+        If the added artist has no transform set to it previously, its
+        transform will be set to ``transFigure``.


figure.transFigure?

I think ideally this docstring would be numpydoc compliant. Most of figure.py is not, but can't hurt to make this new one compliant.

jklymak · 2018-05-11T23:14:56Z

lib/matplotlib/figure.py

+        If the added artist has no transform set to it previously, its
+        transform will be set to ``transFigure``.
+
+        Use ``add_artist`` only in cases you really need to add an artist to


This has a lot of "only" and "most often". Note a big reason to do this is if you don't want the artist in the tighbbox of the axes. Maybe:

Usually artists are added to axes objects using :meth:matplotlib.axes.Axes.add_artist, but use this method in the rare cases that adding directly to the figure is necessary.

timhoffm · 2018-05-14T20:28:51Z

doc/users/next_whats_new/fig_add_artist_method.rst

+
+In case the added artist has no transform set previously, it will assume
+set it the figure transform (``fig.transFigure``).
+This new mathod may be useful for adding artists to figures without axes or to


timhoffm · 2018-05-14T20:30:12Z

doc/users/next_whats_new/fig_add_artist_method.rst

+    fig.add_artist(circ)
+
+In case the added artist has no transform set previously, it will assume
+set it the figure transform (``fig.transFigure``).


Broken sentence

timhoffm · 2018-05-14T20:32:27Z

lib/matplotlib/figure.py

+            transform set to it previously, its transform will be set to
+            ``figure.transFigure``.
+        clip : bool, optional, default ``False``
+            An optional parameter ``clip`` may be used to determine whether the


Shorter: „Determines whether ...“

ImportanceOfBeingErnest · 2018-05-14T21:25:16Z

It doesn't seem like the failing test is due to anything commited here.

jklymak · 2018-05-14T21:31:58Z

You might need to rebase....? Looks like #11204 monkeyed w/ boilerplate... ping @anntzer

ImportanceOfBeingErnest · 2018-05-14T21:59:49Z

Rebasing didn't change anything.

tacaswell · 2018-05-15T03:01:32Z

try running boilerplate.py and commit the results.

On the plus side, this means that test is working! On the down side it is a bit annoying....

tacaswell · 2018-05-15T03:16:40Z

I suspect what happened is that between when the tests on #11204 ran and it got merged something that changed pyplot got merged into master.

efiring

I think the 'clip' kwarg can be removed.

efiring · 2018-05-16T01:34:52Z

lib/matplotlib/figure.py

+            The artist to add to the figure. If the added artist has no
+            transform set to it previously, its transform will be set to
+            ``figure.transFigure``.
+        clip : bool, optional, default ``False``


I don't understand this parameter for a Figure artist; I would not expect that anything could be drawn outside the Figure patch boundaries. I did a little test that seemed to confirm that even with the ps backend, so there is white space around the figure, a Figure.figimage is clipped, regardless of whether get_clip_on() returns True or False.

What if you do savefig(bbox_inches='tight')? If clipping is on, it'll clip the tight bbox (I think, or maybe it will when some of my PRs on this get merged). If clipping is off the figure will get larger...

efiring · 2018-05-16T01:36:29Z

doc/users/next_whats_new/fig_add_artist_method.rst

@@ -0,0 +1,16 @@
+Figure has an `~.figure.Figure.add_artist` method


"an" -> "a"

The ~ cuts it to just add_artist, so 'an' is correct here.

QuLogic · 2018-05-16T03:51:07Z

doc/users/next_whats_new/fig_add_artist_method.rst

@@ -0,0 +1,16 @@
+Figure has an `~.figure.Figure.add_artist` method


The ~ cuts it to just add_artist, so 'an' is correct here.

QuLogic · 2018-05-16T03:51:30Z

doc/users/next_whats_new/fig_add_artist_method.rst

+-------------------------------------------------
+
+A method `~.figure.Figure.add_artist` has been added to the
+:class:`~.figure.Figure` class, which allows to add artists directly to a


allows to add artists -> allows artists to be added

QuLogic · 2018-05-16T03:52:14Z

lib/matplotlib/figure.py

+        ----------
+        artist : `~matplotlib.artist.Artist`
+            The artist to add to the figure. If the added artist has no
+            transform set to it previously, its transform will be set to


set to it previously -> previously set

QuLogic · 2018-05-16T03:54:25Z

lib/matplotlib/figure.py

+            artist.set_transform(self.transFigure)
+
+        if clip:
+            artist.set_clip_path(self.patch)


This should always be passed, unless you can guarantee that the clip path was always False, or it should default to None if you really don't want to change the existing setting. That is, assuming it has an effect (re. @efiring's comment).

Actually, I think I misread this, and thought clip was being passed down. Since it isn't, this doesn't really matter.

ImportanceOfBeingErnest · 2018-05-16T18:42:31Z

The difference between clip=False and clip=True can be observed when saving the figure with the bbox_inches="tight" option.

Default (i.e. clip=False)

import matplotlib.pyplot as plt
plt.rcParams["figure.figsize"] = (3,2)
plt.rcParams["figure.dpi"] = 72

fig, ax = plt.subplots()
c = plt.Circle((0,0),0.2)
fig.add_artist(c)
fig.savefig("test1.png", facecolor="azure", bbox_inches="tight")

With clip=True:

If people think this should be handled differently, I'm happy to change it. Unfortunately I'm not quite sure I understood the comments above. ~~@efiring and~~ @QuLogic, could you explain a bit more in detail what you have in mind concerning the clip option?

My potential objections have been shown to be invalid.

jklymak · 2018-07-05T15:26:38Z

This seems done, and can/should go in for 3.0. Needs a rebase and another approver...

ImportanceOfBeingErnest · 2018-07-05T19:09:53Z

I rebased. There was still a comment by @QuLogic, which I did not fully understand; so maybe he wants to take another look and either approve or clarify that issue?

ImportanceOfBeingErnest · 2018-07-14T10:28:05Z

I needed to rebase yet again; now there is an error from circle/ci saying document not included in any doctree.

jklymak · 2018-07-14T14:00:09Z

Yeah, unfortunately while the release candidate for 3.0 is cut, the whats-new should be moved to the actual whats-new.rst.

ImportanceOfBeingErnest · 2018-07-14T16:39:47Z

Now I get Analysis Failed (could not build the merge commit), but I looking at the log, I don't see what's failing (which is probably due to me not knowing what to look for in this long log file).

anntzer · 2018-07-16T16:45:51Z

lib/matplotlib/tests/test_figure.py

@@ -381,6 +382,34 @@ def test_warn_cl_plus_tl():
    assert not(fig.get_constrained_layout())


+def test_add_artist():


You may want to (but don't have to) use #11408 for the test, which has the benefit of directly taking care of all three output formats and generating failure diffs when needed.

Ok, did that. Locally, the svg test always skips though. Not sure about the reason.

You just don't have inkscape installed. I wouldn't worry about it...

I have several inkscape versions installed, maybe that's the problem.

...sorry, shouldn't have assumed; rephrase - inkscape wasn't callable for some reason 😉

jklymak reviewed May 11, 2018

View reviewed changes

ImportanceOfBeingErnest force-pushed the fig-add-artist branch 2 times, most recently from 9969fc4 to f1046f6 Compare May 12, 2018 00:02

dstansby added this to the v3.0 milestone May 12, 2018

jklymak approved these changes May 14, 2018

View reviewed changes

timhoffm reviewed May 14, 2018

View reviewed changes

ImportanceOfBeingErnest force-pushed the fig-add-artist branch from f1046f6 to ec6df1d Compare May 14, 2018 20:52

ImportanceOfBeingErnest force-pushed the fig-add-artist branch from ec6df1d to 2bdffec Compare May 14, 2018 21:36

anntzer mentioned this pull request May 15, 2018

Re-run boilerplate.py. #11251

Merged

6 tasks

ImportanceOfBeingErnest force-pushed the fig-add-artist branch from 2bdffec to 1ab9b49 Compare May 15, 2018 08:57

efiring previously requested changes May 16, 2018

View reviewed changes

QuLogic reviewed May 16, 2018

View reviewed changes

ImportanceOfBeingErnest force-pushed the fig-add-artist branch from 1ab9b49 to 00e0ac4 Compare May 16, 2018 19:32

jklymak added the status: needs review label May 24, 2018

jklymak added status: needs rebase Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. labels Jul 5, 2018

ImportanceOfBeingErnest force-pushed the fig-add-artist branch 2 times, most recently from 1158162 to 6024bf3 Compare July 5, 2018 19:08

jklymak requested a review from QuLogic July 13, 2018 22:54

ImportanceOfBeingErnest force-pushed the fig-add-artist branch 2 times, most recently from e60a0f3 to 0ade33e Compare July 13, 2018 23:23

ImportanceOfBeingErnest force-pushed the fig-add-artist branch from 0ade33e to 5d208e9 Compare July 14, 2018 14:56

jklymak removed the status: needs rebase label Jul 16, 2018

anntzer reviewed Jul 16, 2018

View reviewed changes

ImportanceOfBeingErnest force-pushed the fig-add-artist branch from 5d208e9 to e4e5a0b Compare July 16, 2018 21:29

Add fig.add_artist method

b6326cc

ImportanceOfBeingErnest force-pushed the fig-add-artist branch from e4e5a0b to b6326cc Compare July 16, 2018 21:41

tacaswell approved these changes Jul 19, 2018

View reviewed changes

tacaswell merged commit ac58f1f into matplotlib:master Jul 19, 2018

QuLogic removed the status: needs review label Jul 20, 2018

ImportanceOfBeingErnest deleted the fig-add-artist branch February 17, 2019 23:08

		@@ -0,0 +1,16 @@
		Figure has an `~.figure.Figure.add_artist` method

		@@ -381,6 +382,34 @@ def test_warn_cl_plus_tl():
		assert not(fig.get_constrained_layout())


		def test_add_artist():

Uh oh!

Add fig.add_artist method #11234

Add fig.add_artist method #11234

Uh oh!

Conversation

Uh oh!

PR Summary

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!