Skip to content

More cleanup to annotations tutorial. #20531

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

Closed
wants to merge 1 commit into from
Closed

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jun 27, 2021

Make most of the tutorial a comment, rather than a string. Remove
non-informative plot titles (as in #20393).

Move annotate_simple01 into the tutorial, and delete it as independent
example.

Remove the table of fancybox properties, and directly output them as
part of the plot in fancybox_demo.py. In that same example, it is
also nicer to use two column output, and simpler to just output one
fancybox per subplot, rather than having to recompute the position of
each fancybox in data coordinates.

Don't alphabetically sort boxstyles/arrowstyles/etc., the source order
being semantically more meaningful (as in #20430).

PR Summary

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

Make most of the tutorial a comment, rather than a string.  Remove
non-informative plot titles.

Move annotate_simple01 into the tutorial, and delete it as independent
example.

Remove the table of fancybox properties, and directly output them as
part of the plot in fancybox_demo.py.  In that same example, it is
also nicer to use two column output, and simpler to just output one
fancybox per subplot, rather than having to recompute the position of
each fancybox in data coordinates.

Don't alphabetically sort boxstyles/arrowstyles/etc., the source order
being semantically more meaningful.
Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

Deleting annotate_simple01 seems somewhat arbitrary. annotate_simple01...04 sort of belong together. They show some examples of annotations. Whether we want them as separate examples (because the show up in the gallery) or whether that's too much can be discussed. But removing only 01 is inconsistent. Either we keep all, or we remove all.

My first-step intuition is to merge annotate_simple01..04 into a single example with a (2, 2) subplot. That way it's still possible to see the variants in the gallery, but they can be described and compared more easily within a single example.

@@ -15,17 +17,16 @@
# First we'll show some sample boxes with fancybox.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# First we'll show some sample boxes with fancybox.
# The following box styles are available:

ax.set_axis_off()
for ax, (stylename, stylecls) in zip(axs.T.flat, styles.items()):
ax.text(.5, .5,
f"{stylename}\n{inspect.signature(stylecls)}",
Copy link
Member

Choose a reason for hiding this comment

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

I'm not convinced this stylecls inspection is an improvement. This plot should primarily illustrate the available boxstyles. Adding the parameters is rather distracting. And also these are only the defaults, which should be irrelevant to most users (well maybe if you start modifying them, but that should be discussed separately.

@anntzer
Copy link
Contributor Author

anntzer commented Jun 28, 2021

Sorry, I think I'm trying to do too much here, let me split that PR into smaller ones...

@story645
Copy link
Member

Sorry I didn't see this before I started #23606 - how badly does that conflict w/ this & should I incorporate stuff from here into there?

@anntzer
Copy link
Contributor Author

anntzer commented Sep 26, 2022

Feel free to pick up whatever is relevant from my patch and either close this if you think everything that matters has been handled, or leave it open if there's more to do.

@QuLogic
Copy link
Member

QuLogic commented Dec 13, 2022

@story645 did you incorporate any of this PR? Or should we close this PR as outdated?

@story645
Copy link
Member

I think I went in a different but overlapping direction, enough that the rebase would be messy. Not sure what @anntzer wants to do w/ the other files in this PR, especially since I moved the code for the user demo examples into the tutorial and therefore think those examples should be removed.

@anntzer
Copy link
Contributor Author

anntzer commented Dec 13, 2022

I'll try and sort out if there's anything left to do and if so open separate PRs for them. I guess this can be closed, in the meantime.

@anntzer anntzer closed this Dec 13, 2022
@anntzer anntzer deleted the ad branch December 14, 2022 09:31
@anntzer
Copy link
Contributor Author

anntzer commented Dec 14, 2022

OK, I think the only remaining work is #24723, everything else has been handled.

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.

4 participants