Update for checking whether colors have an alpha channel by landoskape · Pull Request #27327 · matplotlib/matplotlib

landoskape · 2023-11-15T08:06:07Z

PR summary

Update check of whether a color has an alpha channel to include hexadecimal and color/alpha tuple formats.
Add test of the new formats
Add method to check whether a sequence of colors has an alpha channel

For more explanation see the matplotlib page on specifying colors.

PR checklist

"Closes [Bug]: The method for checking whether a color has an alpha value is outdated #27321"
new and changed code is tested
New Features and API Changes are noted with a directive and release note
Documentation complies with general and docstring guidelines

landoskape · 2023-11-15T08:18:23Z

I'm not sure how to add the check for whether c is indicating a color in a color cycle. I think it requires checking whether the first element of the string is 'C' and if the next elements represent a digit.

Also: I'm not sure why those tests are failing, but it seems like the test server isn't connected to the internet..?

oscargus

I think it makes sense to make this more general. Some comments to consider.

lib/matplotlib/colors.py

oscargus · 2023-11-15T08:30:48Z

lib/matplotlib/colors.py

-    return not isinstance(c, str) and len(c) == 4
+    # If c is not a color, it doesn't have an alpha channel
+    if not is_color_like(c):
+        return False


One should probably check the code if this has already been called. Since this is a private method one can require that it is checked.

Hm, I'm sure I know how to do that or what you mean. Would you explain a little more?

(also, thanks for the comments, as always)

Co-authored-by: Oscar Gustafsson <oscar.gustafsson@gmail.com>

ksunden · 2023-11-21T18:46:55Z

Looks like the CI failures were largely due to network problems on GHA/Azure so cycling to rerun

landoskape · 2023-12-04T09:30:28Z

Background

I opened this PR because of my work on PR #27304, which I though would benefit from a check on whether the user provided a color with an alpha value to determine how the violinplot function handles alpha values. However, after @story645 explained, the point of allowing alpha to be included in a color was to remove unnecessary arguments, which I agree with, so that sort of makes my work here less relevant (except for the fact that the previous _has_alpha_channel() was out of date).

Current use of `_has_alpha_channel()`

The _has_alpha_channel() method is only used in one place throughout the entire library: lib/matplotlib/axis.Tick. In that location, it just checks whether the provided grid color has an alpha channel specified, and if not, uses mpl.rcParams["grid.alpha"] instead.

Tick() is the parent of XTick() and YTick(), which are the parents of SkewXTick(). However, nowhere in the entire library do those methods set the kwargs grid_color or grid_alpha.

Proposal

It seems like _has_alpha_channel() is obsolete, and should be removed from the library. I propose:

Removing it (and it's tests in test_colors.py).
In Tick(), removing grid_alpha as an argument and letting grid_color specify the alpha setting.

timhoffm · 2023-12-04T14:13:13Z

I'm +0.5 on removing grid_alpha. There's value in reducing the number of parameters an dependency between them. While we should not remove primary alpha parameters from functions because they are used too widely, removing *_alpha parameters for secondary color attributes (typically pass through) seems viable.

Before we can remove grid_alpha, it has to go through our deprecation cycle.

It should also be ensured, that grid_color=(None, 0.5) works and only modifies the alpha. Otherwise, only modifying the alpha becomes cumbersome.

timhoffm

I suggest to take the fixed _has_alpha_channel() as minimal bugfix and defer the discussion whether grid.alpha (the only use of _has_alpha_channel()) should be deprecated. Also defer the decision on whether we need _has_alpha_channel_array().

timhoffm · 2024-10-07T13:39:03Z

lib/matplotlib/colors.py

-    """Return whether *c* is a color with an alpha channel."""
-    # 4-element sequences are interpreted as r, g, b, a
-    return not isinstance(c, str) and len(c) == 4
+    """Return whether *c* is a color with an alpha channel"""


We should mention that the result is undefined if c is not a valid color specifier.

This is now mentioned in the docstring and the comment for the final return line.

timhoffm · 2024-10-07T13:41:44Z

lib/matplotlib/colors.py

+    return False
+
+
+def _has_alpha_channel_array(cseq):


This is not used anywhere right now. Therefore, we don't need it here.

It is now deprecated (I added the standard _api.deprecated decorator). In addition I added a deprecation file in the doc/ folder.

Um, you've just introduced this function and immediately deprecated it?!? You should simply not add it to the PR.

rcomer · 2024-12-28T19:18:36Z

@landoskape are you still interested in working on this?

landoskape · 2024-12-28T22:33:55Z

Ah, yes! Lost track of it but I can continue in early January. Thanks for the reminder.

landoskape · 2025-01-03T09:52:11Z

See my responses to @timhoffm. I believe this is all that is needed now since you suggested deferring gridalpha discussion to another time?

lib/matplotlib/colors.py

timhoffm · 2025-01-04T14:20:37Z

lib/matplotlib/colors.py

+    return False
+
+
+def _has_alpha_channel_array(cseq):


Um, you've just introduced this function and immediately deprecated it?!? You should simply not add it to the PR.

rcomer

I suggest starting by checking whether we have a valid color with is_color_like and if not return False. Then the remaining logic can be confident about how c is structured.

lib/matplotlib/colors.py

landoskape · 2025-01-04T17:15:05Z

Ah - @timhoffm good point! The function was introduced because it would have helped the violinplot update that is in PR #27304. It's now removed entirely.

landoskape · 2025-01-04T17:20:13Z

I suggest starting by checking whether we have a valid color with is_color_like and if not return False. Then the remaining logic can be confident about how c is structured.

Smart. I incorporated this.

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

Co-authored-by: Ruth Comer <10599679+rcomer@users.noreply.github.com>

timhoffm

Sorry, after going through the logic carefully, I've some additional remarks. (Could/should have been part of the previous review).

lib/matplotlib/colors.py

lib/matplotlib/tests/test_colors.py

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

landoskape · 2025-01-05T00:04:43Z

Of course, no worries. In agreement with all and suggestions all accepted.

lib/matplotlib/colors.py

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

rcomer · 2025-01-06T16:21:34Z

I do not know what the doc failure is about, but it cannot possibly be caused by this change since we are only modifying a private function.

rcomer · 2025-01-06T16:54:10Z

Thanks @landoskape and congratulations on your first PR merged into Matplotlib! We hope to hear from you again.

landoskape · 2025-01-06T21:20:12Z

Woohoo! Thanks :)

Update _has_alpha_channel

0586383

oscargus reviewed Nov 15, 2023

View reviewed changes

oscargus added the topic: color/alpha label Nov 15, 2023

landoskape and others added 2 commits November 15, 2023 09:00

Update lib/matplotlib/colors.py

9719b60

Co-authored-by: Oscar Gustafsson <oscar.gustafsson@gmail.com>

Clarify comment explanation

77e759f

ksunden closed this Nov 21, 2023

ksunden reopened this Nov 21, 2023

Remove unnecessary check about is_color_like

5d7edca

timhoffm mentioned this pull request Dec 7, 2023

Fixed _has_alpha_channel function to work with hex and tuple colors #27463

Closed

5 tasks

timhoffm reviewed Oct 7, 2024

View reviewed changes

update docstring for undefined case

e7220c8

github-actions bot added the topic: color/color & colormaps label Jan 3, 2025

Deprecate unused method

0cf3612

timhoffm reviewed Jan 4, 2025

View reviewed changes

rcomer reviewed Jan 4, 2025

View reviewed changes

lib/matplotlib/colors.py Outdated Show resolved Hide resolved

landoskape and others added 5 commits January 4, 2025 18:12

Remove unnecessary method

e2099e6

Flake formatting update

5ac2c0f

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

Don't specify what an undefined result does

212363e

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

Recursive check which handles hex colors too

73127eb

Co-authored-by: Ruth Comer <10599679+rcomer@users.noreply.github.com>

Whitespace

c3cf8d4

timhoffm reviewed Jan 4, 2025

View reviewed changes

lib/matplotlib/colors.py Outdated Show resolved Hide resolved

lib/matplotlib/colors.py Outdated Show resolved Hide resolved

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

landoskape and others added 3 commits January 5, 2025 00:03

Remove is_color_like check and explain

b81786c

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

Don't discuss undefined results

80d422d

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

Remove test of undefined result

bbe6fa2

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

timhoffm approved these changes Jan 5, 2025

View reviewed changes

lib/matplotlib/colors.py Outdated Show resolved Hide resolved

landoskape and others added 2 commits January 6, 2025 15:45

Spelling error

de5e42d

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

improved grammar in comment

0813e13

rcomer approved these changes Jan 6, 2025

View reviewed changes

rcomer added this to the v3.11.0 milestone Jan 6, 2025

rcomer merged commit 9bb047d into matplotlib:main Jan 6, 2025

melissawm moved this from Waiting for author to Merged in First Time Contributors Mar 6, 2026

Uh oh!

Conversation

landoskape commented Nov 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR summary

PR checklist

Uh oh!

landoskape commented Nov 15, 2023

Uh oh!

oscargus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ksunden commented Nov 21, 2023

Uh oh!

landoskape commented Dec 4, 2023

Background

Current use of _has_alpha_channel()

Proposal

Uh oh!

timhoffm commented Dec 4, 2023

Uh oh!

timhoffm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rcomer commented Dec 28, 2024

Uh oh!

landoskape commented Dec 28, 2024

Uh oh!

landoskape commented Jan 3, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rcomer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

landoskape commented Jan 4, 2025

Uh oh!

landoskape commented Jan 4, 2025

Uh oh!

timhoffm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

landoskape commented Jan 5, 2025

Uh oh!

Uh oh!

rcomer commented Jan 6, 2025

Uh oh!

rcomer commented Jan 6, 2025

Uh oh!

landoskape commented Jan 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

landoskape commented Nov 15, 2023 •

edited

Loading

Current use of `_has_alpha_channel()`

rcomer left a comment •

edited

Loading