Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Updated Angles on Bracket arrow styles example to make angles clear #23176#24145

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

Merged
QuLogic merged 3 commits intomatplotlib:mainfromjacoverster:jaco-doc-update
Oct 26, 2022

Conversation

jacoverster
Copy link
Contributor

PR Summary

PR relates to thisissue. AddedAngleAnnotation patches to mark the angles and includedFancyArrowPatch arrows to show the directions of AngleA and AngleB.

PR Checklist

Tests and Styling

  • Has pytest style unit tests (andpytest passes).
  • IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).

@story645
Copy link
Member

story645 commentedOct 12, 2022
edited
Loading

I don't think the what's new should be updated mostly cause I don't think this will be very discoverable there and also cause I think we try to avoided updating the what's new docs.

Can you please make this an example in annotations and then link the API docs for arrows to it?@timhoffm may have a better suggestion but I'm not sure what the policy is in terms of these kinds of figures in the reference docs.

ETA: Thanks so much for making this figure and putting in the PR, I'm way psyched for it's existence even if I'm being a grump on placement.

@timhoffm
Copy link
Member

I wouldn't have a problem updating "what's new" in this case. This is "only" a visual improvement of an existing plot.

However, I agree that it's not very discoverable in there.

Can you please make this an example in annotations and then link the API docs for arrows to it?@timhoffm may have a better suggestion but I'm not sure what the policy is in terms of these kinds of figures in the reference docs.

Fundamentally, illustrating plots are fine the reference docs, see e.g. the notes section ofhttps://matplotlib.org/devdocs/api/ticker_api.html#matplotlib.ticker.ScalarFormatter. However the example code is quite long and would clutter the docstring. So, placing it in the text/annotations section of the examples and linking in the plot via.. plot:: /path/to/example athttps://matplotlib.org/devdocs/api/_as_gen/matplotlib.patches.ArrowStyle.html#matplotlib.patches.ArrowStyle is the best solution here.

story645 and jacoverster reacted with thumbs up emoji

@jacoverster
Copy link
ContributorAuthor

Hi@timhoffm and@story645 I've updated the PR with the proposed changes. I created a new example and linked it to matplotlib.patches.ArrowStyle in the example references. I also referenced the example in the ArrowStyle docstring. When I build the documentation it shows up as an example but it does not pull through tohttps://matplotlib.org/devdocs/api/_as_gen/matplotlib.patches.ArrowStyle.html#matplotlib.patches.ArrowStyle - I assume I'm missing something here so please assist with that.

@jklymak
Copy link
Member

You've squashed some unrelated commits in here. An easy way to fix is to copy the two files you wanted to change somewhere else, On your branch dogit fetch upstream; git reset --hard upstream/main and then copy your two files back to where they belong and make your commit. Alternately, you can dogit restore -s upstream/main pathTo/MyFile for the two files.

@jacoverster
Copy link
ContributorAuthor

jacoverster commentedOct 17, 2022
edited
Loading

Hi@jklymak, sorry about that wat trying to follow the process toamend the previous commit. Did not notice the other commit.

git commit --amend --no-editgit push [your-remote-repo] [your-branch] --force-with-lease

@jacoverster
Copy link
ContributorAuthor

You've squashed some unrelated commits in here. An easy way to fix is to copy the two files you wanted to change somewhere else, On your branch dogit fetch upstream; git reset --hard upstream/main and then copy your two files back to where they belong and make your commit. Alternately, you can dogit restore -s upstream/main pathTo/MyFile for the two files.

Hi@jklymak, not sure how that commit got entangled with mine. I tried this without avail, received the following:

fatal: 'upstream' does not appear to be a git repositoryfatal: Could not read from remote repository.

I went ahead and synced my branch with matplotlib:main from GitHub ("sync fork") and then pulled again but now it looks like some of the changes are already merged, so I'm a bit confused. Not sure what to do now.

Please assist and sorry for the mess.

@jklymak
Copy link
Member

You need to add a "remote" calledupstream before you can do that. Our git guide is quite bad, in my opinion. This one is quite good (and I still think we should just copy it, and stop dragging our feet making a new one):https://docs.xarray.dev/en/stable/contributing.html#getting-started-with-git

@jacoversterjacoversterforce-pushed thejaco-doc-update branch 2 times, most recently from1b726d0 to4a3537bCompareOctober 17, 2022 17:58
@jacoverster
Copy link
ContributorAuthor

Hi@jklymak, thanks I managed to set the upstream and then I ran:

git fetch upstreamgit reset --hard upstream/main

Then made the changes again on my branch and ran:

git commit --amend --no-editgit push --force-with-lease

I think it should be good now. Please confirm.

@jklymak
Copy link
Member

Looks better. Congrats!

@story645story645 mentioned this pull requestOct 19, 2022
from matplotlib.patches import Arc, FancyArrowPatch
from matplotlib.transforms import Bbox, IdentityTransform, TransformedBbox


Copy link
Member

Choose a reason for hiding this comment

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

does importing AngleAnnotation from angle_annotation.py work?

Copy link
ContributorAuthor

@jacoversterjacoversterOct 19, 2022
edited
Loading

Choose a reason for hiding this comment

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

Importing AngleAnnotation works but the plots from that example is also automatically generated in the docs then. Not sure if there is a way to exclude them?

Copy link
Member

Choose a reason for hiding this comment

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

hmm, wonder if the trick there is to factor just the AngleAnnotation out into it's own .py file that gets excluded from the sphinx gallery listings and then use a literal block to show it in the scale invariant tutorial.

Comment on lines 3123 to 3129
Examples
--------
.. plot:: gallery/text_labels_and_annotations/angles_on_bracket_arrows.py
Copy link
Member

@story645story645Oct 19, 2022
edited
Loading

Choose a reason for hiding this comment

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

do we usually call this section examples or is notes to be consistent with the other example?

Copy link
ContributorAuthor

@jacoversterjacoversterOct 19, 2022
edited
Loading

Choose a reason for hiding this comment

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

I noticed "Examples" in some docstrings, and "Notes" in others. Seemed like "Examples" was more prevalent. This change did not add the mini gallery to the docs page. Is this the way to do that?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry I spaced - I think notes or examples is contextual and here this plot is a note about how angles work more than an example of how to use brackets

Copy link
Member

@story645story645Oct 19, 2022
edited
Loading

Choose a reason for hiding this comment

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

Also according to our docs, the mini gallery should have been registered as part of the referenceshttps://matplotlib.org/devdocs/devel/documenting_mpl.html#references-for-sphinx-gallery

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Sorry I spaced - I think notes or examples is contextual and here this plot is a note about how angles work more than an example of how to use brackets

Got it, thanks. Will update tot a Note.

story645 reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Also according to our docs, the mini gallery should have been registered as part of the referenceshttps://matplotlib.org/devdocs/devel/documenting_mpl.html#references-for-sphinx-gallery

Is anything else missing@story645? Any ideas why it is not registered?

@story645
Copy link
Member

story645 commentedOct 23, 2022
edited
Loading

So I apologize profusely for doing this now, but I have an idea that might simplify things? Maybe instead of making this a new example (w/ all the annoyances of copying the ArcAnnotation class), this gets folded into the ArcAnnotation example as a second example? And then we title that example something like annotating angles - direction and scale invariance? B/C the direction stuff is what's really useful here as an example, and that the figure makes a good bracket reference is kind of independent...(and yes this then is maybe a tutorial, but that's out of scope here)

You can still keep the link to the figure in the reference using the sphinx gallery reference tagshttps://sphinx-gallery.github.io/stable/advanced.html#cross-referencing

@oscargus
Copy link
Member

The failing test doesn't seem related to this PR:

/home/circleci/project/doc/api/prev_api_changes/api_changes_3.6.0/removals.rst:157: WARNING: py:obj reference target not found: SubplotBase.get_subplotspec/home/circleci/project/doc/api/prev_api_changes/api_changes_3.6.0/removals.rst:158: WARNING: py:obj reference target not found: SubplotBase.set_subplotspec

@story645story645 self-assigned thisOct 23, 2022
@jacoverster
Copy link
ContributorAuthor

jacoverster commentedOct 24, 2022
edited
Loading

So I apologize profusely for doing this now, but I have an idea that might simplify things? Maybe instead of making this a new example (w/ all the annoyances of copying the ArcAnnotation class), this gets folded into the ArcAnnotation example as a second example? And then we title that example something like annotating angles - direction and scale invariance? B/C the direction stuff is what's really useful here as an example, and that the figure makes a good bracket reference is kind of independent...(and yes this then is maybe a tutorial, but that's out of scope here)

You can still keep the link to the figure in the reference using the sphinx gallery reference tagshttps://sphinx-gallery.github.io/stable/advanced.html#cross-referencing

Good suggestion. Alternatively, we could also leave out AngleAnnotation completely (which basically just provides the angle size in this case) and only add the directional arrow. That's what we are really after from the start right, more clarity on the angle direction?

It would then look like this:

image

story645 reacted with thumbs up emoji

@story645
Copy link
Member

story645 commentedOct 24, 2022
edited
Loading

Alternatively, we could also leave out AngleAnnotation completely

I'd still like labeling of the angle between bracket and perpendicular, but that can probably be achieved w/ ax.text placed relative to the perpendicular.

Also I really like your suggestion!

@jacoverster
Copy link
ContributorAuthor

Alternatively, we could also leave out AngleAnnotation completely

I'd still like labeling of the angle between bracket and perpendicular, but that can probably be achieved w/ ax.text placed relative to the perpendicular.

Also I really like your suggestion!

Either one is cool, please let me know what the preferred way is 👌🏻

@story645
Copy link
Member

story645 commentedOct 24, 2022
edited
Loading

I agree w/ you on leaving out ArcAnnotation, (I really like that it's a simpler way to label angles) but if you're gonna go that route please label the angle with ax.text instead.

@jacoverster
Copy link
ContributorAuthor

Ok, so I've simplified the example significantly by removing AngleAnnotation and adding ax.Text angles. The plot looks like this now:
image

In the end it's exactly the same output as with AngleAnnotation, just a lot less code.

story645 reacted with thumbs up emoji

@story645
Copy link
Member

story645 commentedOct 25, 2022
edited
Loading

build failed 'cause of unexpected indent but I'm having trouble finding it in the .py file

/home/circleci/project/doc/gallery/text_labels_and_annotations/angles_on_bracket_arrows.rst:32: ERROR: Unexpected indentation.

eta: probably this line going by the rendered docs
image

@jacoverster
Copy link
ContributorAuthor

build failed 'cause of unexpected indent but I'm having trouble finding it in the .py file

/home/circleci/project/doc/gallery/text_labels_and_annotations/angles_on_bracket_arrows.rst:32: ERROR: Unexpected indentation.

eta: probably this line going by the rendered docsimage

Ah, missed a sneaky space there. Should be fixed now. Sorry about that.

story645 reacted with thumbs up emoji

@story645
Copy link
Member

Sorry about that

No problem/apology needed/wish it was easier to find the errors in the logs :/

Copy link
Member

@story645story645 left a comment

Choose a reason for hiding this comment

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

The return after the image is weird here but an artifact of docstring interpolation and therefore out of scope for this PR.

Screenshot_20221025-161244.jpg

@story645story645 added this to thev3.6.2 milestoneOct 25, 2022
@story645story645 linked an issueOct 25, 2022 that may beclosed by this pull request
@story645
Copy link
Member

story645 commentedOct 25, 2022
edited
Loading

Um that first commit needs to be removed before I merge this. Can you try rebasing against main and dropping that first commit?

If you need help let somebody (me) know!

@QuLogic
Copy link
Member

Thanks@jacoverster! Congratulations on your first PR to Matplotlib 🎉 We hope to hear from you again.

@jacoversterjacoverster deleted the jaco-doc-update branchOctober 26, 2022 11:02
@jacoverster
Copy link
ContributorAuthor

Thanks@jacoverster! Congratulations on your first PR to Matplotlib 🎉 We hope to hear from you again.
It was a pleasure. Thanks for the guidance@story645 and@jklymak. Would love to contribute more.

story645 reacted with heart emoji

story645 added a commit that referenced this pull requestOct 26, 2022
…145-on-v3.6.xBackport PR#24145 on branch v3.6.x (Updated Angles on Bracket arrow styles example to make angles clear#23176)
melissawm pushed a commit to melissawm/matplotlib that referenced this pull requestDec 19, 2022
…atplotlib#23176 (matplotlib#24145)* removed AngleAnnotation from angle_on_bracket_arrow example* Fixes indentation mistake.* rebase to main, remove conflicting commit
@ksundenksunden mentioned this pull requestFeb 20, 2023
6 tasks
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@QuLogicQuLogicQuLogic left review comments

@story645story645story645 approved these changes

Assignees

@story645story645

Labels
None yet
Projects
None yet
Milestone
v3.6.2
Development

Successfully merging this pull request may close these issues.

[Doc]: Bracket ArrowStyle Angle Unclear
6 participants
@jacoverster@story645@timhoffm@jklymak@oscargus@QuLogic

[8]ページ先頭

©2009-2025 Movatter.jp