-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Issue26074: Resolves different edgecolor and hatch color in bar plot #26683
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
The code can be edited in the following way to provide a different hatchcolor and edgecolor |
This change has caused some test failures. @Vashesh08 are you confident to investigate those yourself? You can see the full output from the tests by clicking the "Details" link by one of the checks at the bottom of this page. You can also run the tests yourself locally: |
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.
Is hatch.color only going to work for bar? I think it either needs to work for all patches or at the very least the rcparam needs to be more specific.
lib/matplotlib/axes/_axes.py
Outdated
@@ -2488,6 +2491,12 @@ def bar(self, x, height, width=0.8, bottom=None, *, align="center", | |||
itertools.cycle(mcolors.to_rgba_array(edgecolor)), | |||
# Fallback if edgecolor == "none". | |||
itertools.repeat('none')) | |||
if hatchcolor is None: | |||
hatchcolor = itertools.repeat(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.
This appears to advance the default color so by default the hatch is different from the edge color? That doesn't seem a reasonable default.
Same here, and I think from what I can follow this PR adds a new method to patch to do so and updates the patch constructor was updated and so there should be a test that the keyword is forwarded correctly for all the methods that forward patch arguments. |
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.
really psyched that you took the "add it to the patch constructor" approach! Please:
- check that the functions that return patch objects (like pie's wedges and histograms) can take this argument
- add some tests that this argument works the way it's expected to- defaults to edgecolor or rcparam or uses the passed in color
- add a versionadded note and entry to what's new - links are in the PR checklist
- update the mypy stubs with the new argument and its type
Thanks!
Cool I'll try to run the tests.
@story645 @jklymak rcParams["hatch.color"] is by default black. I'm trying to put the update the hatch color along with the patch arguments. So I am trying to work on this but it will either default to black or I'll have to change the default of the parameter here. |
I've tried a couple of methods. This works for both if we use in the following way
However, for custom hatchcolor in each method, as @story645 mentioned, we need to add it to each method. |
Thanks super helpful. I'll try to work on these. |
So please let us know what your preference is, but I want to ping @timhoffm as API consistency lead. The question here is on defaulting hatchcolor. Current behavior I think is that it defaults to edgecolor and then falls back to hatch.color but I'm not positive and is this the behavior we want? I might also just put this on the call. |
In my opinion it should default to edgecolor. I'm not even clear that an rcParam is a good idea for this parameter - default to edgecolor except if specified would be simplest. |
I'm not sure about this but what I was thinking is that if there are multiple subplots, maybe then instead of specifying it individually, this parameter could be useful.
Like In the above example, I think it should default to hatch.color (because it is set by the user). My suggestion would be if there is no hatchcolor parameter in the function, then it should default to the hatch.color(only if it is set by the user) and maybe then fall back to the edgecolor parameter. |
I think the rcParam is for the edgecolor=None but still want a hatch case, which I think is this example https://matplotlib.org/devdocs/gallery/lines_bars_and_markers/filled_step.html#sphx-glr-gallery-lines-bars-and-markers-filled-step-py which should be much simpler if this PR goes in? It's also possibly all the fill methods? And possibly all the patches - trying to chase through but default seems to be edgecolor=None
Problem is there's no way to distinguish between default black and user set black |
I guess it's OK to have an rcParam if |
Yes, without changing the default,it seems difficult and changing the default would create a lot of problems. I think for the time being, defaulting to edgecolor seems to be the way forward. I'll default it to edgecolor and then to hatch.color. |
@Vashesh08 I think you didn't add/push your tests? |
If you consider the other colors (edge/face) they all store an matplotlib/lib/matplotlib/patches.py Lines 340 to 356 in e65a4e3
I think that a similar approach should be used here so that This also opens up the question if |
I have added a workaround to set the hatchcolor directly from the Patch object, so that hatchcolor is applied on all functions which return patch object. The current behavior is explicity set hatchcolor -> defaults to edgecolor -> defaults to rcParams.
In my opinion, it shouldn't but changing would cause a regression for users. |
PR summary
Fixes #26074
Added a new parameter hatchcolor in bar method which changes the hatch color and also added support for rcParams['hatch.color'] = "color"
Credits to @story645 for coming up with the original idea on how to resolve this.
PR checklist