_check_color_like function for list inputs#25025
Conversation
story645
left a comment
There was a problem hiding this comment.
To fix the flake errors, I suggest installing the pre-commit hooks to more easily find em.
| For each *key, lst* pair in *kwargs*, check that every element *v* in *lst* is color-like. | ||
| """ | ||
| for k, lst in kwargs.items(): | ||
| invalid_colors = list(filter(lambda v: not is_color_like(v), lst)) |
There was a problem hiding this comment.
I think this is super clever and looks like you can use itertools.filterfalse directly.
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
| """ | ||
| for k, lst in kwargs.items(): | ||
| invalid_col = list((lambda v: not is_color_like(v), lst)) | ||
| invalid_col = [c for c in lst if not is color_like(c)] |
There was a problem hiding this comment.
| invalid_col = [c for c in lst if not is color_like(c)] | |
| invalid_col = [c for c in lst if not is_color_like(c)] |
|
It seems you can actually convert "none" to a colour: so I think you just need to pick a different bad colour for your tests. |
|
At this point, I'm not sure what's causing the test to keep failing, I assume there's something about re-escaping in regex that I don't understand. E AssertionError: Regex pattern did not match. |
|
Could be wrong, but I wonder if it's like #24403 and you need a similar escape type thing, something like ETA: I tried this on my machine and I think it worked |
Co-authored-by: Ruth Comer <10599679+rcomer@users.noreply.github.com>
|
|
||
|
|
||
| def test_check_color_like(): | ||
| err_msg = "['abcd'] is not a valid value for c" |
There was a problem hiding this comment.
I'm pretty sure this error message isn't a list because the input to this function isn't a list
|
|
||
|
|
||
| def test_check_color_like_list(): | ||
| err_msg = escape("['abcd'] are not valid values for c") |
There was a problem hiding this comment.
You should be able to use r" instead of escape but if using escape then please write re.escape and import re rather than from re import escape
|
Is there a place to add this and use it to the codebase as well? I worry that we are adding a private method that is never used and then someone will come and clean it up if there is no use case for this in the current code. |
|
I wanted it for #24849, which hasn't gone in (yet) but likely there are other places. |
|
I agree with @greglucas I think this should go in where it is used. I'm not understanding the need for this in the abstract. Note that we do not typically test private methods. Sorry this had a lot of back and forth - but I'm not particularly in favour of this going in with no application. |
|
I spun this off into its own issue so that I wouldn't suggest @rcomer implement this as part of her PR. The specific use case is parity with the gap color implementation that uses the singular case. Edit: while we don't typically test private methods, I don't think it hurts to validate that a validation function isn't broken. |
|
For setting colours on collections, we use In [1]: from matplotlib.colors import to_rgba_array
In [2]: to_rgba_array("octarine")
---------------------------------------------------------------------------
<snip>
ValueError: 'octarine' is not a valid color value.For a list of colours, I agree it could be more helpful: In [3]: to_rgba_array(["skybluepink", "octarine"])
---------------------------------------------------------------------------
<snip>
ValueError: Invalid RGBA argument: 'skybluepink'So I would be inclined to use this new helper within |
|
Somewhat in agreement with various comments above, I think that this should not exist as an independent function at all, and to_rgba_array already provides the desired functionality. (Alternatively, consistently with is_color_like, we could have is_colors_like (plural) returning True/False, but I think the point here is more about the error message?) We could consider making to_rgba_array emit an error message with all the incorrect values, but I'm also quite opposed to that, because that list could be arbitrarily long and there isn't much of a point in spamming the user with hundreds of invalid values. Again this is consistent with CPython itself, see similar discussion at #24889. |
I think flagging once "hey check all your input" is more user friendly than a script that can take however long erroring out however many times it takes for the user to figure out that they've got hundreds of invalid values. Also 'to_rgba' doesn't say which kwarg argument this was passed in through and I think that's really useful for the tracing. |
The error message could easily be something like "At least one invalid RGBA argument: ..." or "Invalid RGBA argument: ... (further values may be invalid too)".
I would grow the signature to |
I like this suggestion but at that point does it make sense to also replace _check_color_like since I think almost everything hits to_rgba or to_rgba_array anyway? basically my initial motivation was to have a consistent validation path for colors and arrays of colors - I'm not overly particular about whether the recommended path is always convert to RGBA or pass through via _check_color* and then convert... If this is too much for @i-deal then I can open a new PR w/ i-deal as a co-author (or maybe make changes on this branch but the GitHub app doesn't tell me what branch this was made on) |
|
I wouldn't mind killing _check_color_like too. |

PR Summary
This PR addresses issue (#25005) by creating a function that validates color inputs in a list format ex. check_color_like_list(colors=['red', 'green', 'yellow']).
This function uses the is_color_like(v) function to determine if each color in each list is a valid color input. Currently, it can take multiple lists of colors as inputs, I am not sure if this is necessary or not.
PR Checklist
Documentation and Tests
Has pytest style unit tests (and pytest passes)
Documentation is sphinx and numpydoc compliant (the docs should build without error).