NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.4k

Add fig.add_artist method#11234

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

tacaswell merged 1 commit intomatplotlib:masterfromImportanceOfBeingErnest:fig-add-artist

Jul 19, 2018

Merged

Add fig.add_artist method#11234

tacaswell merged 1 commit intomatplotlib:masterfromImportanceOfBeingErnest:fig-add-artist

Jul 19, 2018

Conversation

Copy link

Member

ImportanceOfBeingErnest commentedMay 11, 2018•
edited
Loading

PR Summary

As quickly discussedon gitter, anadd_artist() ofFigure 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 reviewed

May 11, 2018

View reviewed changes

Copy link

Member

jklymak left a comment

Choose a reason for hiding this comment

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

Looks great asside frm the small formatting issues! Thanks!

lib/matplotlib/figure.py Outdated

		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``.

Copy link

Member

Choose a reason for hiding this comment

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

figure.transFigure?

Copy link

Member

Choose a reason for hiding this comment

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

I think ideally this docstring would be numpydoc compliant. Most offigure.py isnot, but can't hurt to make this new one compliant.

lib/matplotlib/figure.py Outdated

		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

Copy link

Member

Choose a reason for hiding this comment

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

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.

ImportanceOfBeingErnest force-pushed thefig-add-artist branch 2 times, most recently from9969fc4 tof1046f6Compare

May 12, 2018 00:02

dstansby added this to thev3.0 milestone

May 12, 2018

jklymak approved these changes

May 14, 2018

View reviewed changes

timhoffm reviewed

May 14, 2018

View reviewed changes

doc/users/next_whats_new/fig_add_artist_method.rst Outdated


		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

Copy link

Member

Choose a reason for hiding this comment

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

method

doc/users/next_whats_new/fig_add_artist_method.rst Outdated

		fig.add_artist(circ)

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

Copy link

Member

Choose a reason for hiding this comment

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

Broken sentence

lib/matplotlib/figure.py Outdated

		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

Copy link

Member

Choose a reason for hiding this comment

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

Shorter: „Determines whether ...“

ImportanceOfBeingErnest force-pushed thefig-add-artist branch fromf1046f6 toec6df1dCompare

May 14, 2018 20:52

Copy link

MemberAuthor

ImportanceOfBeingErnest commentedMay 14, 2018

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

Copy link

Member

jklymak commentedMay 14, 2018

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

ImportanceOfBeingErnest force-pushed thefig-add-artist branch fromec6df1d to2bdffecCompare

May 14, 2018 21:36

Copy link

MemberAuthor

ImportanceOfBeingErnest commentedMay 14, 2018

Rebasing didn't change anything.

Copy link

Member

tacaswell commentedMay 15, 2018

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....

Copy link

Member

tacaswell commentedMay 15, 2018

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.

anntzer mentioned this pull request

May 15, 2018

Re-run boilerplate.py.#11251

Merged

6 tasks

ImportanceOfBeingErnest force-pushed thefig-add-artist branch from2bdffec to1ab9b49Compare

May 15, 2018 08:57

efiring previously requested changes

May 16, 2018

View reviewed changes

Copy link

Member

efiring left a comment

Choose a reason for hiding this comment

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

I think the 'clip' kwarg can be removed.

lib/matplotlib/figure.py Outdated

		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``

Copy link

Member

Choose a reason for hiding this comment

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

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.

Copy link

Member

jklymakMay 16, 2018

Choose a reason for hiding this comment

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

What if you dosavefig(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...

doc/users/next_whats_new/fig_add_artist_method.rst Outdated

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

Copy link

Member

efiringMay 16, 2018

Choose a reason for hiding this comment

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

"an" -> "a"

Copy link

Member

QuLogicMay 16, 2018

Choose a reason for hiding this comment

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

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

QuLogic reviewed

May 16, 2018

View reviewed changes

doc/users/next_whats_new/fig_add_artist_method.rst Outdated

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

Copy link

Member

QuLogicMay 16, 2018

Choose a reason for hiding this comment

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

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

doc/users/next_whats_new/fig_add_artist_method.rst Outdated

		-------------------------------------------------

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

Copy link

Member

QuLogicMay 16, 2018

Choose a reason for hiding this comment

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

allows to add artists -> allows artists to be added

lib/matplotlib/figure.py Outdated

		----------
		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

Copy link

Member

QuLogicMay 16, 2018

Choose a reason for hiding this comment

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

set to it previously -> previously set

lib/matplotlib/figure.py

		artist.set_transform(self.transFigure)

		if clip:
		artist.set_clip_path(self.patch)

Copy link

Member

QuLogicMay 16, 2018

Choose a reason for hiding this comment

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

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

Copy link

Member

QuLogicJul 13, 2018

Choose a reason for hiding this comment

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

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

Copy link

MemberAuthor

ImportanceOfBeingErnest commentedMay 16, 2018•
edited
Loading

The difference betweenclip=False andclip=True can be observed when saving the figure with thebbox_inches="tight" option.

Default (i.e. clip=False)

import matplotlib.pyplot as pltplt.rcParams["figure.figsize"] = (3,2)plt.rcParams["figure.dpi"] = 72fig, ax = plt.subplots()c = plt.Circle((0,0),0.2)fig.add_artist(c)fig.savefig("test1.png", facecolor="azure", bbox_inches="tight")

Withclip=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 theclip option?

efiring dismissed theirstale review

May 16, 2018 19:00

My potential objections have been shown to be invalid.

ImportanceOfBeingErnest force-pushed thefig-add-artist branch from1ab9b49 to00e0ac4Compare

May 16, 2018 19:32

jklymak added the status: needs review label

May 24, 2018

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

Jul 5, 2018

Copy link

Member

jklymak commentedJul 5, 2018

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

ImportanceOfBeingErnest force-pushed thefig-add-artist branch 2 times, most recently from1158162 to6024bf3Compare

July 5, 2018 19:08

Copy link

MemberAuthor

ImportanceOfBeingErnest commentedJul 5, 2018

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?

jklymak requested a review fromQuLogic

July 13, 2018 22:54

ImportanceOfBeingErnest force-pushed thefig-add-artist branch 2 times, most recently frome60a0f3 to0ade33eCompare

July 13, 2018 23:23

Copy link

MemberAuthor

ImportanceOfBeingErnest commentedJul 14, 2018

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

Copy link

Member

jklymak commentedJul 14, 2018

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

ImportanceOfBeingErnest force-pushed thefig-add-artist branch from0ade33e to5d208e9Compare

July 14, 2018 14:56

Copy link

MemberAuthor

ImportanceOfBeingErnest commentedJul 14, 2018

Now I getAnalysis 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).

jklymak removed the status: needs rebase label

Jul 16, 2018

anntzer reviewed

Jul 16, 2018

View reviewed changes

lib/matplotlib/tests/test_figure.py Outdated

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


		def test_add_artist():

Copy link

Contributor

anntzerJul 16, 2018

Choose a reason for hiding this comment

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

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.