Skip to content

Documentation for using ConnectionPatch across Axes with constrained_… #14957

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 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
Aug 14, 2019
Merged

Documentation for using ConnectionPatch across Axes with constrained_… #14957

merged 3 commits into from
Aug 14, 2019

Conversation

canismarko
Copy link

@canismarko canismarko commented Aug 1, 2019

…layout.

When using a figure with contrained layout management, adding a
ConnectionPatch between different subplot Axes breaks the layout. The
artist should be removed from layout considerations before being added
to the axes. This commit notes this fix in the documentation.

#14907

PR Summary

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

…layout.

When using a figure with contrained layout management, adding a
ConnectionPatch between different subplot Axes breaks the layout. The
artist should be removed from layout considerations before being added
to the axes. This commit notes this fix in the documentation.

#14907
# :class:`~matplotlib.axes.Axes` must be removed from the layout
# using :meth:`~matplotlib.artist.Artist.set_in_layout` before
# calling :meth:`~matplotlib.axes.Axes.add_artist`. See
# :class:`~matplotlib.patches.ConnectionPatch` for more information.

Choose a reason for hiding this comment

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

In principle this applies to just any artist that uses axes coordinates and is added to an axes. So similar problems are expected for annotations or annotation boxes in axes coordinates, ax{h,v}lines if they overflow the axes boundary, legends (which are actually mentionned someplace earlier in this tutorial), etc.

Maybe there is a way to formulate this more generally and name the connection patch as an example?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah sure. I updated the constrained_layout tutorial to apply to all artists. Let me know what you think.

Choose a reason for hiding this comment

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

Contentwise I'd be fine with that. I'll leave it to a native speaker to check if that sentence makes sense grammatically. Probably, @jklymak, the original author of that tutorial would like to have a look at it anyways.

@jklymak
Copy link
Member

jklymak commented Aug 4, 2019

I am away from computer for a few days. If folks think this is an improvement don’t wait for me!

@ImportanceOfBeingErnest
Copy link
Member

Or maybe one should advocate the use of fig.add_artist() more in this respect? (Artists added to figures do not generally take part in the layout)
That would include:

  • mention it in the tight layout and constrained layout tutorial
  • mention it in the docstring for artists that are often positionned outside axes, like ConnectionPatch, but also AnnotationBbox, Annotation
  • use it in the Connect Simple01 example. Also, the Demo Annotation Box

@anntzer
Copy link
Contributor

anntzer commented Aug 4, 2019

I agree that using Figure.add_artist makes more sense than adding to the Axes and then excluding from the Axes layout...

@canismarko
Copy link
Author

Alright, I will update the PR to suggest Figure.add_artist instead. Thanks.

@ImportanceOfBeingErnest
Copy link
Member

Except that fig.add_artist(ConnectionPatch(..)) doesn't currently work. So one would need to make it work first.

@ImportanceOfBeingErnest
Copy link
Member

#15020 would make it work. Once it's merged documentation can use it.

@canismarko
Copy link
Author

Since #15020 was merged a few days ago, I revised the PR to recommend using Figure.add_artist().

@tacaswell
Copy link
Member

Looks good to me, leaving to @ImportanceOfBeingErnest to merge.

Copy link
Member

@ImportanceOfBeingErnest ImportanceOfBeingErnest left a comment

Choose a reason for hiding this comment

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

I meantionned a few points in above (#14957 (comment)) which would help making the option of using fig.add_artist more discoverable for users when using layout managers.

This PR is consistent in itself and tackles some of them. So I will approve and merge.
Still further PRs, featuring the other points are still very welcome.

@ImportanceOfBeingErnest ImportanceOfBeingErnest merged commit b34c605 into matplotlib:master Aug 14, 2019
@tacaswell
Copy link
Member

Thank you @canismarko and congratulations on your first Matplotlib PR 🎉 Hopefully we will hear from you again!

@canismarko
Copy link
Author

Thanks, @tacaswell. I'll see if I can implement the rest of the suggestions from @ImportanceOfBeingErnest in #14957 (comment) and open a new PR when they're done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants