-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Separates edgecolor from hatchcolor #28104
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
Conversation
|
There are two tests that are currently failing and need a rewrite.
|
|
Pinging @story645 for review |
|
Sorry for delay, will try to review but please add some tests. |
|
Also this should probably go in before #27937 b/c I agree that it'll probably clean that one up. |
story645
left a comment
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 might need some discussion on how to manage since the changes dive pretty deep into the internals, but definitely add some tests cause that could help you debug the segfault.
|
For this change we need to make sure both:
This will also need a lot of documentation (both notifying third-party backends the expected API is changing) and announcing the new feature to users (with lots of examples). |
|
Could I also go ahead with rewriting the two tests that are failing? |
It shouldn't be failing. This is what @tacaswell was talking about that you want to make sure the code is backwards compatible. The test contour_hatch also seems like it should still pass, is there a reason it shouldn't? |
The initial comment explains the reason |
|
Sorry, I updated while you posted. For the tests:
I think the user specified alpha should also be propagated to hatchcolors, w/ the same priority rules as color and edgecolor, which should then make the test pass.
|
|
It may be simpler to work out what the non-vectorized version of this API looks like (so do just the classes in |
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
72d47a7 to
a23c9c5
Compare
story645
left a comment
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.
slight wording nits but otherwise I think this is good to go and a feature I wanted a few weeks back
| if self._edgecolor[3] == 0: # fully transparent | ||
| return colors.to_rgba(mpl.rcParams['patch.edgecolor']) | ||
| return self.get_edgecolor() |
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'm gonna move the discussion of deprecating to a new issue, but my concern is that the fallback on edgecolor='None' means:
- if edgecolor='None' you'll backfill with 'patch.edgecolor' but that may also be None and this all happens silently
- edgecolor=('green', 0) will also trigger this path and the person may have wanted the 'green' and were gonna up the alpha
|
Thanks @Impaler343 for going with us through the lengthy discussion and design process. |
PR summary
Taking after @Vashesh08 's PR #26993
Should close #26074 and #7059
Implemented the fallback logic that @story645 mentioned in this comment
Cherry picked some initial commits from the Issue26074 branch written by Vashesh
Modified the hatchcolor setting to way similar to edgecolor setting.
PR checklist