Documentation for using ConnectionPatch across Axes with constrained_… by canismarko · Pull Request #14957 · matplotlib/matplotlib · GitHub
Skip to content

Documentation for using ConnectionPatch across Axes with constrained_… - #14957

Merged
ImportanceOfBeingErnest merged 3 commits into
matplotlib:masterfrom
canismarko:conn-patch-docs
Aug 14, 2019
Merged

ImportanceOfBeingErnest merged 3 commits into
matplotlib:masterfrom
canismarko:conn-patch-docs

Conversation

@canismarko

@canismarko canismarko commented Aug 1, 2019

Copy link
Copy Markdown

…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

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

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

jklymak commented Aug 4, 2019

Copy link
Copy Markdown
Member

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

@ImportanceOfBeingErnest

Copy link
Copy Markdown
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

anntzer commented Aug 4, 2019

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Author

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

@ImportanceOfBeingErnest

Copy link
Copy Markdown
Member

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

@ImportanceOfBeingErnest

Copy link
Copy Markdown
Member

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

@canismarko

Copy link
Copy Markdown
Author

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

@tacaswell

Copy link
Copy Markdown
Member

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

@ImportanceOfBeingErnest ImportanceOfBeingErnest left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
Member

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

@canismarko

Copy link
Copy Markdown
Author

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