-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
19195 rotated markers #20914
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
19195 rotated markers #20914
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks 👍 overall. I've left some comments and questions.
1a6f754
to
1aef5fa
Compare
@deep-jkl this has developed a merge conflict. I don't think that should affect his reviewable it is, but will need to be fixed. It looks like some others have put some time into this, so feel free to ping them again if it is ready for a new look. Thanks for your patience! |
1aef5fa
to
75a92d8
Compare
Improvement was done in instantiating new instance utilizing deep copy should preserve immutability of MarkerStyle members (e.g. Path, or Transform).
Added, translated, scaled, and rotated methods with test.
Apply suggestion from @timhoffm, update lib/matplotlib/markers.py Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Thanks for the reminder, I hoped that you were just busy with the release. @timhoffm @dstansby I have resolved conflict and comments, can you have a look on this PR? |
@deep-jkl thanks for following up. It'll be a couple of days before I can have a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good to me. I made a bunch of suggestions, mostly to do w/ the doc strings...
Apply suggestions from @jklymak code review Co-authored-by: Jody Klymak <jklymak@gmail.com>
Apply suggestions from code review Co-authored-by: Jody Klymak <jklymak@gmail.com>
Thanks for review, I have processed all the suggestions so far. |
Check for user style in get_cap(join)style is removed. This is also supported with two new tests. Additional test checks that get_transform returns combination of supplied and internal transformation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, and is an oft-requested feature that I think will make a lot of people happy!
19195 rotated markers
When these Enum classes were added in matplotlib#18544, they were supposed to be for documentation only. To that end, matplotlib#22055 was a followup that ensured that only the strings were exposed from the getter side. However, when user-supplied cap/join style were added in matplotlib#20914, they were only for the Enum type instead of the string, so correctly allow strings here as well. Also, specifically type hint the return values as literals, as was done in matplotlib#25719.
When these Enum classes were added in matplotlib#18544, they were supposed to be for documentation only. To that end, matplotlib#22055 was a followup that ensured that only the strings were exposed from the getter side. However, when user-supplied cap/join style were added in matplotlib#20914, they were only for the Enum type instead of the string, so correctly allow strings here as well. Also, specifically type hint the return values as literals, as was done in matplotlib#25719.
When these Enum classes were added in matplotlib#18544, they were supposed to be for documentation only. To that end, matplotlib#22055 was a followup that ensured that only the strings were exposed from the getter side. However, when user-supplied cap/join style were added in matplotlib#20914, they were only for the Enum type instead of the string, so correctly allow strings here as well. Also, specifically type hint the return values as literals, as was done in matplotlib#25719.
When these Enum classes were added in matplotlib#18544, they were supposed to be for documentation only. To that end, matplotlib#22055 was a followup that ensured that only the strings were exposed from the getter side. However, when user-supplied cap/join style were added in matplotlib#20914, they were only for the Enum type instead of the string, so correctly allow strings here as well. Also, specifically type hint the return values as literals, as was done in matplotlib#25719.
When these Enum classes were added in matplotlib#18544, they were supposed to be for documentation only. To that end, matplotlib#22055 was a followup that ensured that only the strings were exposed from the getter side. However, when user-supplied cap/join style were added in matplotlib#20914, they were only for the Enum type instead of the string, so correctly allow strings here as well. Also, specifically type hint the return values as literals, as was done in matplotlib#25719.
PR Summary
Add user supplied transforms and join/cap styles …
Improvement was done in instantiating new instance utilizing deep copy should preserve immutability of MarkerStyle members (e.g. Path, or Transform). However deepcopy of MarkerStyle(r"$|||$") fails. Originally it was needed in lines.py, but I think it can be optimized out to avoid multiple re-instantiation of MarkerStyle.
I followed the discussion for issue #19195 and tried several cases:
Things that needs to be done:
PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
and runflake8 --docstring-convention=all
).doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).