FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

_check_color_like function for list inputs by i-deal · Pull Request #25025 · matplotlib/matplotlib · GitHub

Repository navigation

_check_color_like function for list inputs - #25025

Closed
i-deal wants to merge 23 commits into
matplotlib:mainfrom
i-deal:main
Closed

i-deal wants to merge 23 commits into
matplotlib:mainfrom
i-deal:main

Conversation

i-deal commented Jan 18, 2023

Copy link
Copy Markdown

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

i-deal marked this pull request as ready for review January 18, 2023 23:17

Copy link
Copy Markdown
Member

Currently, it can take multiple lists of colors as inputs,

I think it's good for it to match the color like API and I really really like that you collect the wrong colors and print them as a list rather than failing at first invalid. I think that can save so much headache down the line.

story645 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

To fix the flake errors, I suggest installing the pre-commit hooks to more easily find em.

Comment thread lib/matplotlib/colors.py Outdated
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))

story645 Jan 19, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think this is super clever and looks like you can use itertools.filterfalse directly.

Comment thread lib/matplotlib/colors.py Outdated
i-deal and others added 3 commits January 19, 2023 16:52
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Comment thread lib/matplotlib/colors.py Outdated
"""
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)]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
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)]

rcomer linked an issue Jan 21, 2023 that may be closed by this pull request

rcomer commented Jan 21, 2023

Copy link
Copy Markdown
Member

It seems you can actually convert "none" to a colour:

>>> from matplotlib import colors
>>> colors.to_rgba("none")
(0.0, 0.0, 0.0, 0.0)

so I think you just need to pick a different bad colour for your tests.

Comment thread lib/matplotlib/tests/test_colors.py Outdated

i-deal commented Jan 22, 2023

Copy link
Copy Markdown
Author

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.
E Regex: "['abcd'] are not valid value(s) for c"
E Input: "['abcd'] are not valid value(s) for c"
E Did you mean to re.escape() the regex?

story645 commented Jan 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Could be wrong, but I wonder if it's like #24403 and you need a similar escape type thing, something like r"\['abcd'\] are not valid value(s) for c"

ETA: I tried this on my machine and I think it worked

rcomer left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sorry this is giving you the run-around @i-deal. I think you are almost there: I tried this suggested change locally and the test passed.

Comment thread lib/matplotlib/colors.py Outdated
Comment thread lib/matplotlib/tests/test_colors.py Outdated
i-deal and others added 2 commits January 24, 2023 17:40
Co-authored-by: Ruth Comer <10599679+rcomer@users.noreply.github.com>
Comment thread lib/matplotlib/colors.py Outdated


def test_check_color_like():
err_msg = "['abcd'] is not a valid value for c"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Member

I wanted it for #24849, which hasn't gone in (yet) but likely there are other places.

jklymak commented Jan 31, 2023

Copy link
Copy Markdown
Member

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.

jklymak added the status: needs clarification Issues that need more information to resolve. label Jan 31, 2023

story645 commented Jan 31, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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.

rcomer commented Jan 31, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

For setting colours on collections, we use to_rgba_array. This function currently throws a good exception if you pass a single wrong colour:

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 to_rgba_array, so that all the various colour setters automatically get the benefit.

anntzer commented Jan 31, 2023

Copy link
Copy Markdown
Contributor

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.

story645 commented Jan 31, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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.

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.

anntzer commented Jan 31, 2023

Copy link
Copy Markdown
Contributor

I think flagging once "hey check all your input" is more user friendly

The error message could easily be something like "At least one invalid RGBA argument: ..." or "Invalid RGBA argument: ... (further values may be invalid too)".

'to_rgba' doesn't say which kwarg argument this was passed in through

I would grow the signature to to_rgba_array(..., errname=...) (name up to bikeshedding) if you want to customize that.

Copy link
Copy Markdown
Member

I would grow the signature to to_rgba_array(..., errname=...) (name up to bikeshedding) if you want to customize that.

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)

anntzer commented Jan 31, 2023

Copy link
Copy Markdown
Contributor

I wouldn't mind killing _check_color_like too.

story645 commented Feb 2, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Hey @i-deal, sorry but consensus is that this isn't gonna go in because folks are happy with to_rgba_array and do not want to add a second validation method. Thank you so much for your work here and sparking a discussion on validation consistency. I'm gonna open up a follow up PR removing _check_color_like and replacing it with to_rgba because I think there will be some discussion of growing the signature and how to do the drop/whether to do a drop in in that I'm concerned may be a little too much for a new contributor to manage, but I will credit you as a coauthor. Thanks again!

ETA: the replacement pr is #25140 and @i-deal please take a look/leave comments/reviews/suggestions.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: needs clarification Issues that need more information to resolve.

Projects

Development

Successfully merging this pull request may close these issues.

[MNT]: _check_color_like for list like input

8 participants


Back | FazBrowse Home | New Git URL