Creating_parse_bar_color_args to unify color handling in plt.bar with precedence and sequence support for facecolor and edgecolor #29133

Lucx33 · 2024-11-13T04:39:24Z

PR summary

This PR introduces the _parse_bar_color_args function, which provides consistency in color bar arguments by establishing a parameter precedence, "similar" to the one used in scatter:

facecolor: facecolor -> color -> 'b' if in classic mode else the result of get_next_color_func()

edgecolor: edgecolor -> color -> None

This ensures that when edgecolors is not explicitly defined, it tries color. Additionally, this change allows facecolor to accept sequences of colors, ensuring consistency in argument handling, as issued in #29090. The tests are consistent with examples in #29072.

Examples:

plt.bar([1, 2, 3], [1, 2, 3], color=['none', 'C2', 'none'], edgecolor=['C1', 'C2', 'C3'])
plt.bar([1, 2, 3], [1, 2, 3], color=['none', 'C2', 'none'])
plt.bar([1, 2, 3], [1, 2, 3])
plt.bar([1, 2, 3], [1, 2, 3],facecolor=('none', 'C2', 'none'))
plt.bar([1, 2, 3], [1, 2, 3], color=('C1', 'C2', 'C3'), facecolor=('none', 'C2', 'none'))

PR checklist

"closes [MNT]: More consistent color parameters for bar() #29090" is in the body of the PR description to link the related issue
new and changed code is tested
Plotting related features are demonstrated in an example
Documentation complies with general and docstring guidelines

Lucx33 · 2024-11-13T05:26:42Z

Although some tests are failing, I can't determine the cause; For example, test_bbox_inches_tight in test_bbox_tight fails, even though old/new images appear equal.

rcomer · 2024-11-13T08:26:54Z

The easiest test to debug might be test_bar_color_cycle in test_axes.py since that one explicitly shows that we are expecting green but getting blue. So, having already plotted a line, bar is not using the same color in the cycle as plot did.

rcomer · 2024-11-14T12:44:23Z

lib/matplotlib/axes/_axes.py

+        edgecolor = kwargs.pop('edgecolor', color)
+
+        facecolor = (facecolor if facecolor is not None
+                 else "b" if mpl.rcParams['_internal.classic_mode']


I guess you made "b" the default in classic mode for consistency with scatter. However this is not the established behaviour for bar and setting it to blue is likely the cause of at least some of your test failures. For scatter it will only be there for consistency with old Matplotlib versions. I think we should just use the get_next_color_func here.

rcomer · 2024-11-14T12:50:03Z

lib/matplotlib/axes/_axes.py

@@ -2320,7 +2320,59 @@ def _convert_dx(dx, x0, xconv, convert):
            # we do by default and convert dx by itself.
            dx = convert(dx)
        return dx
+
+    @staticmethod
+    def _parse_bar_color_args(kwargs, get_next_color_func):


According to #12739 the method for parsing scatter color arguments was made static so that we could have unit tests specifically for that method. Do you plan to write similar tests here? If not, this could be an ordinary method and we would not need to pass the get_next_color_func but just access self._get_patches_for_fill.get_next_color directly.

Edit: looking at the discussion in #12739 I think it is not advisable to write such tests but just test the public bar function.

Lucx33 · 2024-11-14T14:54:37Z

@rcomer thx for the review, I'll fix it as soon as possible. prob later today

Lucx33 · 2024-11-14T22:19:54Z

@rcomer test_bar_color_cycle passed as you said. Others still failed.

lib/matplotlib/axes/_axes.py

rcomer · 2024-11-15T09:23:19Z

Here are the before and after of that test_bbox_inches_tight according to the tests. Note that the image tests run in classic mode unless the style keyword is explicitly set.

Before

After

Difference

So it seems the edgecolor has changed from black to blue.

Note you can download all the test images under "Artifacts" on the Test Summary Page. However those zip files are large so it may be more efficient to run the tests locally and inspect the images you produce that way.

Lucx33 · 2024-11-20T02:05:18Z

@timhoffm thx for the review, commited it.

All tests in test_axes passed except for test_bar_hatches but the problem seems to be the ref image:
ax_ref.bar(x[i], y[i], color='C0', hatch=hatches[i])
The generated image appears as expected (Older version). Changing color='C0' to facecolor='C0' resolves the issue:
ax_ref.bar(x[i], y[i], facecolor='C0', hatch=hatches[i])

I can commit this to the PR if you think it's okay.

Lucx33 · 2024-11-20T02:33:57Z

@rcomer In test_bbox_inches_tight, since our new precedence says that if color is specified, it defines both facecolor and edgecolor ('b' in this case). Changing color to facecolor fixes the issue:
Old: ax.bar(ind, data[row], width, bottom=yoff, align='edge', color='b')
New: ax.bar(ind, data[row], width, bottom=yoff, align='edge', facecolor='b')

Lucx33 · 2024-11-20T02:37:42Z

Other tests fails for the same reason as both tests above.

rcomer · 2024-11-20T09:00:53Z

I do not think we really want edgecolor to match color by default: if I take the grouped bar example and

reverse the order of the bars so the shorter ones are drawn later
make facecolor and edgecolor the same

we see that the shorter bars now overlap the larger ones, which is not visually appealing. This does not happen if the edgecolor is left off

timhoffm · 2024-11-20T09:36:06Z

I do not think we really want edgecolor to match color by default

Indeed. Currently, color is equivalent to facecolor, and it's also documented ("The colors of the bar faces."). We should not change this. Fundamentally, we wouldn't even need color here. facecolor and edgecolor would be enough, but color dates back at least to 2009. Let's just keep it as a synonym for facecolor.

Lucx33 · 2024-11-20T19:09:23Z

@rcomer @timhoffm Removed edgecolor being color by default as you both said. I think this PR is 100% ok now.

rcomer · 2024-11-24T18:31:58Z

@Lucx33 please add a test for the new behaviour.

Lucx33 · 2024-11-24T23:45:53Z

@rcomer Is this test good enought?

timhoffm · 2024-11-25T16:34:09Z

lib/matplotlib/axes/_axes.py

@@ -2378,6 +2428,9 @@ def bar(self, x, height, width=0.8, bottom=None, *, align="center",
        color : :mpltype:`color` or list of :mpltype:`color`, optional
            The colors of the bar faces.


Suggested change

The colors of the bar faces.

The colors of the bar faces. This is an alias for *facecolor*.

If both are given, *facecolor* takes precedence.

timhoffm · 2024-11-25T16:34:46Z

lib/matplotlib/axes/_axes.py

@@ -2378,6 +2428,9 @@ def bar(self, x, height, width=0.8, bottom=None, *, align="center",
        color : :mpltype:`color` or list of :mpltype:`color`, optional
            The colors of the bar faces.

+        facecolor : :mpltype:`color` or list of :mpltype:`color`, optional
+            The colors of the bar faces.


Suggested change

The colors of the bar faces.

The colors of the bar faces.

If both *color* and *facecolor are given, *facecolor* takes precedence.

timhoffm · 2024-11-25T16:34:54Z

lib/matplotlib/tests/test_axes.py

+    for bar in bars:
+        assert to_rgba(bar.get_facecolor()) == to_rgba('red')
+
+    ax.cla()


You don't need to clear in between, which unfortunately is a relatively expensive action. Instead I'd do

# case 1: no color specified bars = ax.bar([1, 2, 3], [4, 5, 6]) [...] # case 2: Only 'color' bars = ax.bar([11, 12, 13], [4, 5, 6], color='red') [...] # case 3: Only 'color' bars = ax.bar([21, 22, 23], [4, 5, 6], color='red') [...] # case 4: 'facecolor' and 'color' bars = ax.bar([1, 2, 3], [4, 5, 6], color='red', facecolor='green') [...]

Putting the no color case first ensures noting is messing with the colorcycle.

Optional: You could use https://matplotlib.org/stable/api/_as_gen/matplotlib.colors.same_color.html#matplotlib.colors.same_color for the asserts.

Lucx33 · 2024-11-26T16:19:39Z

@timhoffm thx for the review, commited it.

lib/matplotlib/tests/test_axes.py

Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>

Lucx33 · 2024-11-27T15:10:01Z

Is there anything else I can do to help with the merge?

timhoffm · 2024-11-27T16:13:25Z

No, we just need a second approval from another core developer.

Note for other reviewers: The win10 test failure is the flaky determinism check and not related to this PR.

rcomer

Thanks @Lucx33, I just have one small comment. Otherwise this looks good to me.

lib/matplotlib/tests/test_axes.py

Lucx33 · 2024-11-30T18:56:46Z

Messed up my commit by changing wrong file manually but fixed already

Lucx33 · 2024-11-30T18:57:45Z

@rcomer Changed blue as you said

…color handling in plt.bar with precedence and sequence support for facecolor and edgecolor

rcomer · 2024-11-30T19:50:09Z

Thanks @Lucx33 and congratulations on your first Matplotlib PR! I hope we hear from you again.

…133-on-v3.10.x Backport PR #29133 on branch v3.10.x (Creating_parse_bar_color_args to unify color handling in plt.bar with precedence and sequence support for facecolor and edgecolor)

Consistent color parameters for bar()

cde62aa

rcomer reviewed Nov 14, 2024

View reviewed changes

Fix static issue

bca9d43

timhoffm reviewed Nov 15, 2024

View reviewed changes

lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved

timhoffm reviewed Nov 15, 2024

View reviewed changes

lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved

timhoffm reviewed Nov 15, 2024

View reviewed changes

lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved

Removed unnecessary to_rgba_array()

ab2253f

Fixed flake8

88bc7a1

Removed edgecolor inheritance

6193498

Added test_bar_color_precedence

94db14e

Fix

f92aa1a

timhoffm reviewed Nov 25, 2024

View reviewed changes

melissawm added Maintenance API: consistency labels Nov 25, 2024

Lucx33 added 2 commits November 26, 2024 13:11

Refactored test_bar_color_precedence

4959889

Update bar

ef746a3

timhoffm approved these changes Nov 26, 2024

View reviewed changes

lib/matplotlib/tests/test_axes.py Outdated Show resolved Hide resolved

Lucx33 and others added 2 commits November 26, 2024 14:47

Update lib/matplotlib/tests/test_axes.py

42f5a71

Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>

Update test_axes.py

314a3dc

rcomer reviewed Nov 30, 2024

View reviewed changes

lib/matplotlib/tests/test_axes.py Outdated Show resolved Hide resolved

Minor change "blue" to "yellow"

062dda1

github-actions bot added the status: needs rebase label Nov 30, 2024

Fix axes

ee98b4f

github-actions bot removed the status: needs rebase label Nov 30, 2024

Minor change "blue" to "yellow"

08741ef

rcomer approved these changes Nov 30, 2024

View reviewed changes

rcomer added this to the v3.10.0 milestone Nov 30, 2024

rcomer merged commit 7025fb9 into matplotlib:main Nov 30, 2024
39 checks passed

meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Nov 30, 2024

Backport PR matplotlib#29133: Creating_parse_bar_color_args to unify …

71c55ab

…color handling in plt.bar with precedence and sequence support for facecolor and edgecolor

meeseeksmachine mentioned this pull request Nov 30, 2024

Backport PR #29133 on branch v3.10.x (Creating_parse_bar_color_args to unify color handling in plt.bar with precedence and sequence support for facecolor and edgecolor) #29211

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Creating_parse_bar_color_args to unify color handling in plt.bar with precedence and sequence support for facecolor and edgecolor #29133

Creating_parse_bar_color_args to unify color handling in plt.bar with precedence and sequence support for facecolor and edgecolor #29133

Lucx33 commented Nov 13, 2024

Lucx33 commented Nov 13, 2024

rcomer commented Nov 13, 2024 •

edited

Loading

rcomer Nov 14, 2024

rcomer Nov 14, 2024 •

edited

Loading

Lucx33 commented Nov 14, 2024

Lucx33 commented Nov 14, 2024

rcomer commented Nov 15, 2024 •

edited

Loading

Lucx33 commented Nov 20, 2024 •

edited

Loading

Lucx33 commented Nov 20, 2024 •

edited

Loading

Lucx33 commented Nov 20, 2024

rcomer commented Nov 20, 2024

timhoffm commented Nov 20, 2024

Lucx33 commented Nov 20, 2024

rcomer commented Nov 24, 2024

Lucx33 commented Nov 24, 2024

timhoffm Nov 25, 2024

timhoffm Nov 25, 2024

timhoffm Nov 25, 2024

Lucx33 commented Nov 26, 2024

Lucx33 commented Nov 27, 2024

timhoffm commented Nov 27, 2024 •

edited

Loading

rcomer left a comment

Lucx33 commented Nov 30, 2024 •

edited

Loading

Lucx33 commented Nov 30, 2024

rcomer commented Nov 30, 2024

		@@ -2378,6 +2428,9 @@ def bar(self, x, height, width=0.8, bottom=None, *, align="center",
		color : :mpltype:`color` or list of :mpltype:`color`, optional
		The colors of the bar faces.

	The colors of the bar faces.
	The colors of the bar faces. This is an alias for facecolor.
	If both are given, facecolor takes precedence.

	The colors of the bar faces.
	The colors of the bar faces.
	If both color and facecolor are given, facecolor* takes precedence.

Creating_parse_bar_color_args to unify color handling in plt.bar with precedence and sequence support for facecolor and edgecolor #29133

Creating_parse_bar_color_args to unify color handling in plt.bar with precedence and sequence support for facecolor and edgecolor #29133

Conversation

Lucx33 commented Nov 13, 2024

PR summary

PR checklist

Lucx33 commented Nov 13, 2024

rcomer commented Nov 13, 2024 • edited Loading

rcomer Nov 14, 2024

Choose a reason for hiding this comment

rcomer Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Lucx33 commented Nov 14, 2024

Lucx33 commented Nov 14, 2024

rcomer commented Nov 15, 2024 • edited Loading

Lucx33 commented Nov 20, 2024 • edited Loading

Lucx33 commented Nov 20, 2024 • edited Loading

Lucx33 commented Nov 20, 2024

rcomer commented Nov 20, 2024

timhoffm commented Nov 20, 2024

Lucx33 commented Nov 20, 2024

rcomer commented Nov 24, 2024

Lucx33 commented Nov 24, 2024

timhoffm Nov 25, 2024

Choose a reason for hiding this comment

timhoffm Nov 25, 2024

Choose a reason for hiding this comment

timhoffm Nov 25, 2024

Choose a reason for hiding this comment

Lucx33 commented Nov 26, 2024

Lucx33 commented Nov 27, 2024

timhoffm commented Nov 27, 2024 • edited Loading

rcomer left a comment

Choose a reason for hiding this comment

Lucx33 commented Nov 30, 2024 • edited Loading

Lucx33 commented Nov 30, 2024

rcomer commented Nov 30, 2024

rcomer commented Nov 13, 2024 •

edited

Loading

rcomer Nov 14, 2024 •

edited

Loading

rcomer commented Nov 15, 2024 •

edited

Loading

Lucx33 commented Nov 20, 2024 •

edited

Loading

Lucx33 commented Nov 20, 2024 •

edited

Loading

timhoffm commented Nov 27, 2024 •

edited

Loading

Lucx33 commented Nov 30, 2024 •

edited

Loading