Skip to content

added offset section & restructured annotations tutorial #23606

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 1 commit into from
Oct 30, 2022

Conversation

story645
Copy link
Member

@story645 story645 commented Aug 12, 2022

PR Summary

Updated the annotation tutorial to hopefully clarify how relative positioning of offsets works. attn @ubitux

ETA: Also ended up moving things around to try and create some conceptual groupings.

ETA 2: I have no idea what the capitalization convention is supposed to be

PR Checklist

Tests and Styling

  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

@story645 story645 changed the title added offset to annotations tutorial added offset section to annotations tutorial Aug 12, 2022
@story645 story645 force-pushed the annotation-tutorial branch from 971b5ef to 9323dee Compare August 12, 2022 03:42
@story645 story645 changed the title added offset section to annotations tutorial added offset section & restructered annotations tutorial Aug 15, 2022
@story645 story645 changed the title added offset section & restructered annotations tutorial added offset section & restructured annotations tutorial Aug 15, 2022
@story645 story645 force-pushed the annotation-tutorial branch 4 times, most recently from 286d738 to ee8b44e Compare August 15, 2022 16:16
@story645 story645 force-pushed the annotation-tutorial branch 2 times, most recently from db4afbb to e5f8bd4 Compare August 16, 2022 00:51
@madphysicist
Copy link
Contributor

General comment, not sure if it's in scope here: the example figures seem to be unrelated to the code snippets in some cases. That's a bit confusing/distracting. Would you be willing to update?

@story645
Copy link
Member Author

Would you be willing to update?

Absolutely if you're willing to flag 'em.

@story645
Copy link
Member Author

story645 commented Sep 11, 2022

@timhoffm , @jklymak , @QuLogic is it better than what was there enough yet?

@madphysicist can you open an issue about which figures don't match which code? Or if not, let me know and I'll do it. I wanted to do it here but honestly lately this tutorial seems to be getting touched a lot and I don't know how many more times I want to rebase. 🙃

Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

LGTM.

I didn't realize you moved the custom box style section, but might as well fix it up now.

@madphysicist
Copy link
Contributor

@story645 Looks like the only place on this page with a discrepancy is the ConnectionPatch section towards the end. What I did not realize at the time is that you can click on a figure to get the code to generate it, so you can probably just ignore the original comment.

@story645
Copy link
Member Author

@madphysicist I thought I fixed that already by changing the coordinates to

xy = (0.3, 0.2)

can you please double check?

@story645
Copy link
Member Author

story645 commented Sep 25, 2022

The more I touch this the more I think we should either inset or scrap the linking to the gallery examples 'cause the lack of mechanism to keep them in sync is causing major drift and like parts of this thing read like they refer to gallery examples that no longer exist. Also very out of scope of this PR.

@story645 story645 marked this pull request as draft September 25, 2022 19:23
@timhoffm
Copy link
Member

I agree, we should not be relying on figures from examples in the tutorials. The tutorials should be self-contained. It's simple enough to make the plots in there (possibly enhanced by optional folding sphinx-gallery/sphinx-gallery#953 in the future for plots whose code is not so important). It's ok however, to link to an example in a "see also" style.

@story645 story645 force-pushed the annotation-tutorial branch from c7f11ff to a8cde8d Compare October 16, 2022 08:26
@story645 story645 marked this pull request as ready for review October 16, 2022 08:26
@story645
Copy link
Member Author

So I decided embedding the examples was in scope, I think mostly cause that was easier than checking for/reconciling discrepancies and there were sections of the tutorial that benefited from having the code written out rather than just linking out to a figure.

I intentionally avoided removing stuff from this tutorial, but what belongs in here is definitely arguable. There's now some repetition of gallery examples, but I think that's okay 'cause this allows for divergence down the line. Also a follow up issue/PR would be to clean up https://matplotlib.org/devdocs/gallery/userdemo/index.html and maybe to bring in the zoom code if code folding gets implement.

@story645
Copy link
Member Author

Another follow up option is maybe switch out some of the remaining source/target figures for the plot directive + show source link when #24215 goes in

@story645
Copy link
Member Author

Ping: I think it qualifies as better than it was if only cause the code examples now line up w/ the figures and the text.

AnchoredOffsetbox section, embedded code that used to link out to
gallery examples, also minor consistency and linking tweaks throughout

Co-authored-by: Jody Klymak <jklymak@gmail.com>
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Co-authored-by: Joseph Fox-Rabinovitz <madphysicist@users.noreply.github.com>
@story645 story645 force-pushed the annotation-tutorial branch from a8cde8d to d784332 Compare October 30, 2022 19:36
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.

I'll merge, because this is better than the current state. There's still room for improvements, but that can be in followup PRs.

@timhoffm timhoffm merged commit 799f735 into matplotlib:main Oct 30, 2022
@story645
Copy link
Member Author

Thank you! Totally agree that this needs more work, I just wanted it off my plate.

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