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

Addresses issue #24618 "Road sign" boxstyle/annotation#24697

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

Closed

Conversation

Abitamim
Copy link

PR Summary

Hello, this is my first contribution to an open-source project. This PR is to address the concerns raised in#24618. I've added the ability to specify fhead in the boxstyle attributes. By specifying "fhead=True" for DArrow, LArrow, or RArrow, this makes the head of the arrow flush with the top and bottom of the body.

I need advise on if this was the correct approach, or if I should try something else.

Also, in the documentation, I do not know how to update the graphic, since it is a static image.

PR Checklist

Documentation and Tests

  • [Unsure] Has pytest style unit tests (andpytest passes)
  • Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).
  • [Need help] New plotting related features are documented with examples.

Release Notes

  • [Unsure] New features are marked with a.. versionadded:: directive in the docstring and documented indoc/users/next_whats_new/
  • [Unsure] API changes are marked with a.. versionchanged:: directive in the docstring and documented indoc/api/next_api_changes/
  • [Unsure] Release notes conform with instructions innext_whats_new/README.rst ornext_api_changes/README.rst

Copy link

@github-actionsgithub-actionsbot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping@matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join uson gitter for real-time discussion.

For details on testing, writing docs, and our review process, please seethe developer guide

We strive to be a welcoming and open project. Please follow ourCode of Conduct.

@oscargus
Copy link
Member

Thank you for this!

I think that there needs to be a bit of discussion if this is the way to go or not. The suggestion from@anntzer was to enable modifying all aspects of the arrow, where this was a special case. I can see that as a quite good option if we start modifying the arrow styles, although your approach clearly solves my use case.

Regarding documentation, the image is generated from "all" default styles during building. Hence, I think what should be done is to add a "What's new note" highlighting the new option. In addition, a test is required. I would simply add all three new styles in a single test image.

However, you may want to wait a bit with this until it is decided what the preferred way forward is.

Finally, one may want to modify the custom annotation demo onhttps://matplotlib.org/devdocs/tutorials/text/annotations.html to show some other shape, as the one in the example can now be obtained without a custom shape. (This should probably be done independent of solution, but not sure what a suitable alternative shape is...)

@Abitamim
Copy link
Author

Abitamim commentedDec 12, 2022
edited
Loading

Thank you for this!

I think that there needs to be a bit of discussion if this is the way to go or not. The suggestion from@anntzer was to enable modifying all aspects of the arrow, where this was a special case. I can see that as a quite good option if we start modifying the arrow styles, although your approach clearly solves my use case.

Regarding documentation, the image is generated from "all" default styles during building. Hence, I think what should be done is to add a "What's new note" highlighting the new option. In addition, a test is required. I would simply add all three new styles in a single test image.

However, you may want to wait a bit with this until it is decided what the preferred way forward is.

Finally, one may want to modify the custom annotation demo onhttps://matplotlib.org/devdocs/tutorials/text/annotations.html to show some other shape, as the one in the example can now be obtained without a custom shape. (This should probably be done independent of solution, but not sure what a suitable alternative shape is...)

I made a change to the table in the tutorial section above the image. Should I not have done that -i.e. would that have also been generated automatically?

Also, where do I add a test?

I will work on a different version of this where the width of the arrowhead is adjustable as a percentage of the width of the body, and the length of the arrowhead is also adjustable by specifying an angle. Is this a better approach? And if so, how should I create a separate pull request for that?

@melissawm
Copy link
Member

Hello@Abitamim - I'm sorry this fell through the cracks. Are you interested in picking this back up?

For this PR, you probably want to add a test here:https://github.com/matplotlib/matplotlib/blob/main/lib/matplotlib/tests/test_patches.py

For the tutorial, I believe that is the right change.

Let us know if you have other questions and if you need help with the rebase that needs to be done here, check ourdocumentation on rebasing on main. Cheers!

@Abitamim
Copy link
Author

Abitamim commentedJun 16, 2023 via email

I’m interested in picking it back u0
________________________________From: Melissa Weber Mendonça ***@***.***>Sent: Thursday, June 15, 2023 12:49:54 PMTo: matplotlib/matplotlib ***@***.***>Cc: Abitamim Bharmal ***@***.***>; Mention ***@***.***>Subject: Re: [matplotlib/matplotlib] Addresses issue#24618 "Road sign" boxstyle/annotation (PR#24697)Hello@Abitamim<https://github.com/Abitamim> - I'm sorry this fell through the cracks. Are you interested in picking this back up?For this PR, you probably want to add a test here:https://github.com/matplotlib/matplotlib/blob/main/lib/matplotlib/tests/test_patches.pyFor the tutorial, I believe that is the right change.Let us know if you have other questions and if you need help with the rebase that needs to be done here, check our documentation on rebasing on main<https://matplotlib.org/devdocs/devel/development_workflow.html#rebasing-on-upstream-main>. Cheers!—Reply to this email directly, view it on GitHub<#24697 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ADDBKOPKVCSM2TSDO4ZOOM3XLNROFANCNFSM6AAAAAAS3KWB6E>.You are receiving this because you were mentioned.Message ID: ***@***.***>

@ksundenksunden mentioned this pull requestJul 21, 2023
5 tasks
@tacaswell
Copy link
Member

Closing in favor of#24744

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@github-actionsgithub-actions[bot]github-actions[bot] left review comments

Assignees
No one assigned
Projects
Status: Waiting for author
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@Abitamim@oscargus@melissawm@tacaswell

[8]ページ先頭

©2009-2025 Movatter.jp