Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
19195 rotated markers#20914
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
1a6f754
to1aef5fa
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
to75a92d8
CompareImprovement was done in instantiating new instanceutilizing deep copy should preserve immutability ofMarkerStyle members (e.g. Path, or Transform).
Added, translated, scaled, and rotated methods with test.
Apply suggestion from@timhoffm, update lib/matplotlib/markers.pyCo-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...
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Apply suggestions from@jklymak code reviewCo-authored-by: Jody Klymak <jklymak@gmail.com>
Apply suggestions from code reviewCo-authored-by: Jody Klymak <jklymak@gmail.com>
Thanks for review, I have processed all the suggestions so far. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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 ofsupplied 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 inmatplotlib#18544, they were supposed to befor documentation only. To that end,matplotlib#22055 was a followup that ensuredthat only the strings were exposed from the getter side.However, when user-supplied cap/join style were added inmatplotlib#20914, theywere only for the Enum type instead of the string, so correctly allowstrings here as well.Also, specifically type hint the return values as literals, as was doneinmatplotlib#25719.
When these Enum classes were added inmatplotlib#18544, they were supposed to befor documentation only. To that end,matplotlib#22055 was a followup that ensuredthat only the strings were exposed from the getter side.However, when user-supplied cap/join style were added inmatplotlib#20914, theywere only for the Enum type instead of the string, so correctly allowstrings here as well.Also, specifically type hint the return values as literals, as was doneinmatplotlib#25719.
When these Enum classes were added inmatplotlib#18544, they were supposed to befor documentation only. To that end,matplotlib#22055 was a followup that ensuredthat only the strings were exposed from the getter side.However, when user-supplied cap/join style were added inmatplotlib#20914, theywere only for the Enum type instead of the string, so correctly allowstrings here as well.Also, specifically type hint the return values as literals, as was doneinmatplotlib#25719.
When these Enum classes were added inmatplotlib#18544, they were supposed to befor documentation only. To that end,matplotlib#22055 was a followup that ensuredthat only the strings were exposed from the getter side.However, when user-supplied cap/join style were added inmatplotlib#20914, theywere only for the Enum type instead of the string, so correctly allowstrings here as well.Also, specifically type hint the return values as literals, as was doneinmatplotlib#25719.
When these Enum classes were added inmatplotlib#18544, they were supposed to befor documentation only. To that end,matplotlib#22055 was a followup that ensuredthat only the strings were exposed from the getter side.However, when user-supplied cap/join style were added inmatplotlib#20914, theywere only for the Enum type instead of the string, so correctly allowstrings here as well.Also, specifically type hint the return values as literals, as was doneinmatplotlib#25719.
Uh oh!
There was an error while loading.Please reload this page.
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).