Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
rcomer merged 14 commits intomatplotlib:mainfromLucx33:main
Nov 30, 2024

Conversation

Lucx33
Copy link
Contributor

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

facecolor: facecolor -> color -> 'b' if in classic mode else the result ofget_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'])
    Figure_1

  • plt.bar([1, 2, 3], [1, 2, 3], color=['none', 'C2', 'none'])
    Figure_2

  • plt.bar([1, 2, 3], [1, 2, 3])Figure_3

  • plt.bar([1, 2, 3], [1, 2, 3],facecolor=('none', 'C2', 'none'))
    Figure_4

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

PR checklist

@Lucx33
Copy link
ContributorAuthor

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

rcomer commentedNov 13, 2024•
edited
Loading

The easiest test to debug might betest_bar_color_cycle intest_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 asplot did.

edgecolor = kwargs.pop('edgecolor', color)

facecolor = (facecolor if facecolor is not None
else "b" if mpl.rcParams['_internal.classic_mode']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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

@rcomerrcomerNov 14, 2024•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

According to#12739 the method for parsingscatter 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 theget_next_color_func but just accessself._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 publicbar function.

@Lucx33
Copy link
ContributorAuthor

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

@Lucx33
Copy link
ContributorAuthor

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

@rcomer
Copy link
Member

rcomer commentedNov 15, 2024•
edited
Loading

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

Before
bbox_inches_tight-expected

After
bbox_inches_tight

Difference
bbox_inches_tight-failed-diff

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

Note you can download all the test images under "Artifacts" on theTest 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
Copy link
ContributorAuthor

Lucx33 commentedNov 20, 2024•
edited
Loading

@timhoffm thx for the review, commited it.

All tests in test_axes passed except fortest_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). Changingcolor='C0' tofacecolor='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
Copy link
ContributorAuthor

Lucx33 commentedNov 20, 2024•
edited
Loading

@rcomer Intest_bbox_inches_tight, since our new precedence says that if color is specified, it defines both facecolor and edgecolor ('b' in this case). Changingcolor tofacecolor 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
Copy link
ContributorAuthor

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

@rcomer
Copy link
Member

I do not think we really wantedgecolor to matchcolor by default: if I takethe 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

image

timhoffm reacted with thumbs up emoji

@timhoffm
Copy link
Member

I do not think we really wantedgecolor to matchcolor by default

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

@Lucx33
Copy link
ContributorAuthor

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

@rcomer
Copy link
Member

@Lucx33 please add a test for the new behaviour.

@Lucx33
Copy link
ContributorAuthor

@rcomer Is this test good enought?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change
Thecolorsofthebarfaces.
Thecolorsofthebarfaces.Thisisanaliasfor*facecolor*.
Ifbotharegiven,*facecolor*takesprecedence.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change
Thecolorsofthebarfaces.
Thecolorsofthebarfaces.
Ifboth*color*and*facecoloraregiven,*facecolor*takesprecedence.

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

ax.cla()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 usehttps://matplotlib.org/stable/api/_as_gen/matplotlib.colors.same_color.html#matplotlib.colors.same_color for the asserts.

@Lucx33
Copy link
ContributorAuthor

@timhoffm thx for the review, commited it.

Lucx33and others added2 commitsNovember 26, 2024 14:47
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
@Lucx33
Copy link
ContributorAuthor

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

@timhoffm
Copy link
Member

timhoffm commentedNov 27, 2024•
edited
Loading

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.

Lucx33 reacted with thumbs up emoji

Copy link
Member

@rcomerrcomer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

@Lucx33
Copy link
ContributorAuthor

Lucx33 commentedNov 30, 2024•
edited
Loading

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

@Lucx33
Copy link
ContributorAuthor

@rcomer Changed blue as you said

@rcomerrcomer added this to thev3.10.0 milestoneNov 30, 2024
@rcomerrcomer merged commit7025fb9 intomatplotlib:mainNov 30, 2024
39 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull requestNov 30, 2024
…color handling in plt.bar with precedence and sequence support for facecolor and edgecolor
@rcomer
Copy link
Member

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

Lucx33 reacted with laugh emoji

ksunden added a commit that referenced this pull requestDec 4, 2024
…133-on-v3.10.xBackport 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)
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@timhoffmtimhoffmtimhoffm approved these changes

@rcomerrcomerrcomer approved these changes

Assignees
No one assigned
Projects
Status: Waiting for author
Milestone
v3.10.0
Development

Successfully merging this pull request may close these issues.

[MNT]: More consistent color parameters for bar()
4 participants
@Lucx33@rcomer@timhoffm@melissawm
[8]ページ先頭
©2009-2025 Movatter.jp