Skip to content
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

Issue26074: Resolves different edgecolor and hatch color in bar plot #26683

Closed
wants to merge 0 commits into from

Conversation

Vashesh08
Copy link
Contributor

@Vashesh08 Vashesh08 commented Sep 3, 2023

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

@Vashesh08
Copy link
Contributor Author

Vashesh08 commented Sep 3, 2023

import matplotlib.pyplot as plt
import numpy as np

width = 0.35
x = np.arange(4) + 1
y_red = [1, 3, 1, 4]
y_blue = [2, 2, 4, 1]

plt.bar(x - 0.5 * width, y_red, width, label="Height of red bars", hatch="////", facecolor=(0, 0, 0, 0), edgecolor="black", hatchcolor="red")

plt.bar(x + 0.5 * width, y_blue, width, label="Height of blue bars", hatch=r"\\", facecolor=(0, 0, 0, 0), edgecolor="black", hatchcolor="blue")

plt.xticks(x)
plt.yticks([0, 1, 2, 3, 4])
plt.legend()
plt.savefig("hatch.png")
plt.show()

The code can be edited in the following way to provide a different hatchcolor and edgecolor

image

@Vashesh08 Vashesh08 changed the title Issue26074 Issue26074: Resolves different edgecolor and hatch color in bar plot Sep 3, 2023
@rcomer
Copy link
Member

rcomer commented Sep 3, 2023

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:
https://matplotlib.org/devdocs/devel/testing.html

Copy link
Member

@jklymak jklymak left a 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.

@@ -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)
Copy link
Member

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.

@story645
Copy link
Member

story645 commented Sep 3, 2023

I think it either needs to work for all patches

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.

Copy link
Member

@story645 story645 left a 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:

  1. check that the functions that return patch objects (like pie's wedges and histograms) can take this argument
  2. add some tests that this argument works the way it's expected to- defaults to edgecolor or rcparam or uses the passed in color
  3. add a versionadded note and entry to what's new - links are in the PR checklist
  4. update the mypy stubs with the new argument and its type
    Thanks!

lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved
@Vashesh08
Copy link
Contributor Author

Vashesh08 commented Sep 4, 2023

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: https://matplotlib.org/devdocs/devel/testing.html

Cool I'll try to run the tests.

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.

@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.
https://github.com/matplotlib/matplotlib/blob/389a7e8889c25d05afab4e9ba7ce43e56cf82a99/lib/matplotlib/mpl-data/matplotlibrc#L159C1-L160C1.
Any suggestions ?

@Vashesh08
Copy link
Contributor Author

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.

I've tried a couple of methods. This works for both if we use in the following way

import matplotlib as mpl
mpl.rcParams["hatch.color"]="color"

However, for custom hatchcolor in each method, as @story645 mentioned, we need to add it to each method.

@Vashesh08
Copy link
Contributor Author

really psyched that you took the "add it to the patch constructor" approach! Please:

  1. check that the functions that return patch objects (like pie's wedges and histograms) can take this argument
  2. add some tests that this argument works the way it's expected to- defaults to edgecolor or rcparam or uses the passed in color
  3. add a versionadded note and entry to what's new - links are in the PR checklist
  4. update the mypy stubs with the new argument and its type
    Thanks!

Thanks super helpful. I'll try to work on these.

@story645
Copy link
Member

story645 commented Sep 5, 2023

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.

@jklymak
Copy link
Member

jklymak commented Sep 5, 2023

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.

@Vashesh08
Copy link
Contributor Author

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.

import matplotlib.pyplot as plt

# formatting
plt.rcParams["hatch.color"] = "yellow"
plt.rcParams["text.usetex"] = True
plt.rc('font', family='serif')
plt.rc("font",size=11)

first_year = [20,10,70]

legend=["First","Second"]
fig1,(ax1, ax2) = plt.subplots(1,2)
ax1.pie(first_year, hatch=['**O', 'oO', 'O.O', '.||.'])
ax1.set_title("Year 1",y=1.5,weight='bold')
fig1.legend(legend,ncol=3,loc="lower center")
data1 = [0.3, 0.4, 0.3]


# plot each pie chart in a separate subplot
ax2.pie(data1, hatch=['**O', 'oO', 'O.O', '.||.'])
ax2.set_title("Year 2",y=1.5,weight='bold')

plt.show()
plt.show()

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.

@story645
Copy link
Member

story645 commented Sep 5, 2023

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 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

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.

Problem is there's no way to distinguish between default black and user set black

@jklymak
Copy link
Member

jklymak commented Sep 5, 2023

I guess it's OK to have an rcParam if edgecolor='none' (not None). But otherwise I think it should default to the edgecolor, and otherwise need to be specified as a kwarg.

@Vashesh08
Copy link
Contributor Author

Vashesh08 commented Sep 6, 2023

Problem is there's no way to distinguish between default black and user set black

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.

@story645
Copy link
Member

@Vashesh08 I think you didn't add/push your tests?

@oscargus
Copy link
Contributor

oscargus commented Sep 13, 2023

If you consider the other colors (edge/face) they all store an _original_edgecolor etc so that alpha can be changed separately. Have a look at

def _set_facecolor(self, color):
if color is None:
color = mpl.rcParams['patch.facecolor']
alpha = self._alpha if self._fill else 0
self._facecolor = colors.to_rgba(color, alpha)
self.stale = True
def set_facecolor(self, color):
"""
Set the patch face color.
Parameters
----------
color : color or None
"""
self._original_facecolor = color
self._set_facecolor(color)

I think that a similar approach should be used here so that set_alpha(0.5) will apply to the hatches as well.

This also opens up the question if set_edgecolor should set the hatch color or not? Or if one possibly can keep track of if hatchcolor is set explicitly (a non-None value) or not. Maybe it can be solved through documenting it carefully though.

@Vashesh08
Copy link
Contributor Author

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.

This also opens up the question if set_edgecolor should set the hatch color or not?

In my opinion, it shouldn't but changing would cause a regression for users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

[ENH]: Different edgecolor and hatch color in bar plot
6 participants