Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Separates edgecolor from hatchcolor #28104
Changes from all commits
7b489af
ed55f09
3155bc0
905ed86
7942afd
219bb26
8e732fa
67a54c3
84e12fb
5b3a1e6
8454dfc
a3c80e0
253c7e4
0754734
a74f2af
91ae826
a23c9c5
4dd0c2d
6b54933
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
so just checking that this will get followed up and just used get_edgecolor? cause like I mentioned, I'm uncomfortable with using transparent edge as the test
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.
discussed this on the call today and is this being done to maintain backward compatibility if someone sets edgecolor to
None
and doesn't definehatchcolor
orhatch.color
to ensure that something gets drawn?my thinking is that that should return a warning "Setting a hatch but hatch and edgecolor are none" but we shouldn't special case a fallback (that also may not work if patch.edgecolor is "none")
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.
Yes. This is for the final fallback where edgecolor is set to 'none', and hatchcolor is not set in which case
patch.edgecolor
rcParam is used and this condition is there to check whetheredgecolor
is None. Removing this condition causestest_artist::test_hatching
to fail.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.
Ok, so we should discuss if this behavior should be deprecated (b/c it's an iffy fallback to rely on facecolor.rcparam being set to not None) now that we allow an explicit hatchcolor, but I think it's okay to push that to a follow up to this PR.
Is part of the problem here eager resolution of edgeolor so you can't check that edgecolor is None (hasn't been set)?
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.
Yes, as
hatchcolor
must depend on the currentedgecolor
now, we are essentially special casing for when thehatchcolor
came through asblack
from the rcParam in the previous implementation. I believe we can't really check if the edgecolor is None in a better placeI am for hatching with the rcParam when no
hatchcolor
is specified as it not only maintains backward compatibility but seems more natural for a user to not care about thehatchcolor
, but the next best thing would be to raise a warning as mentioned aboveThere 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: