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

replaced _check_color_like method with call to to_rgba by story645 · Pull Request #25140 · matplotlib/matplotlib · GitHub

Repository navigation

replaced _check_color_like method with call to to_rgba - #25140

Closed
story645 wants to merge 1 commit into
matplotlib:mainfrom
story645:color-check
Closed

story645 wants to merge 1 commit into
matplotlib:mainfrom
story645:color-check

Conversation

story645 commented Feb 3, 2023

Copy link
Copy Markdown
Member

In #25025 there was opposition to adding a new color checking method for arrays to be consistent with the _check_color_like checks, and in the call today there seemed to be consensus for using to_rgba as the validator, especially since _check_color_like is using it under the hood. _check_color_like was used in 5 places, so just swapped out those calls w/ calls to to_rgba. Didn't do anything w/ the return type mostly because I wasn't sure if we wanted class objects to retain the original input.

To keep the error messages from check_color_like, I went with the suggestion in this comment but didn't document it (yet) b/c I'm not sure if this is really a public keyword.

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

Originally posted by @anntzer in #25025 (comment)

to_rgba signature and error message to optionally support the argument
name

Co-authored-by: i-deal <113572023+i-deal@users.noreply.github.com>
story645 added API: argument checking Validation of acceptable argument values topic: color/colorbar labels Feb 3, 2023

Copy link
Copy Markdown
Member

I am not at all excited about extending the API of to_rgba to add the ability to template the error message in a limited way. That is typically much better done via

try:
    c = to_rgba(...)
except ValueError as e:
    raise ValueError("some custom and more helpful error message with context") from e  # or from None

and we should not be leaking what is really internal details (helpers that existing to reduce the above boiler plate) into the public API.


I am a bit worried that this passed tests as is_color_like has a special cut-out for the 'C2' style colors which can not be evaluated until after we have rcparams fullly setup. We should sort out if we no longer can ever try to do that, if we need to add a test , or be sure if the affected code paths can never be run during start up. The biggest worry here is adding a "fails to import" type bug which the user can not recover from!

rcomer commented Feb 3, 2023

Copy link
Copy Markdown
Member

Didn't do anything w/ the return type mostly because I wasn't sure if we wanted class objects to retain the original input.

FWIW, both Patch and Collection go ahead and set the color as the rgba value, e.g.

def _set_edgecolor(self, color):
set_hatch_color = True
if color is None:
if (mpl.rcParams['patch.force_edgecolor'] or
not self._fill or self._edge_default):
color = mpl.rcParams['patch.edgecolor']
else:
color = 'none'
set_hatch_color = False
self._edgecolor = colors.to_rgba(color, self._alpha)
if set_hatch_color:
self._hatch_color = self._edgecolor
self.stale = True
def set_edgecolor(self, color):
"""
Set the patch edge color.
Parameters
----------
color : color or None
"""
self._original_edgecolor = color
self._set_edgecolor(color)

The original color is retained as a private attribute, but I think only used when updating alpha.

Re the try-except loop: it seems to me that that is basically what _check_color_like is doing, neatly packaged so we don’t need to write it out in multiple places.

story645 commented Feb 3, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

I am a bit worried that this passed tests as is_color_like has a special cut-out for the 'C2' style colors which can not be evaluated until after we have rcparams fullly setup.

to_rgba also has a carve out:

# Special-case nth color syntax because it should not be cached.
if _is_nth_color(c):
prop_cycler = mpl.rcParams['axes.prop_cycle']
colors = prop_cycler.by_key().get('color', ['k'])
c = colors[int(c[1:]) % len(colors)]

And trying to pull the carve out of is_color_like broke the tests, so my guess is it's used in the startup stuff.

Re the try-except loop: it seems to me that that is basically what _check_color_like is doing, neatly packaged so we don’t need to write it out in multiple places.

Yup, and I like it and was just unhappy that lists of colors was using to_rgba_array directly, but it's only used in 5 places and the multiple argument functionality is never used. Basically I just want consistency in color validation, also in where colors are validated, because aside from those I think everything else gets validated in draw time, and some of this has to do w/ inconsistencies in supported args like the auto/none thing happening here.

there is also the decorator approach to hide the internal arg (which will also work for all the to_rgba functions) but I'm worried about runtime:

def error_msg_parameter_name(f, c, errname=None):
    try:
        value = f(c)
    except ValueError as e:
        print(dir(e))
        raise ValueError(str(e) +f'{"is not a valid value for " + errname if errname else "."}')
    return value

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

jklymak commented Feb 24, 2023

Copy link
Copy Markdown
Member

Can we clarify the status of this one?

jklymak marked this pull request as draft June 14, 2023 23:43

jklymak commented Jun 14, 2023

Copy link
Copy Markdown
Member

I've moved to draft.

Copy link
Copy Markdown
Member Author

That's fine, honestly I'd prefer a more consistent approach to validation, probably as part of the rcParams work.

story645 closed this Jun 23, 2023
story645 deleted the color-check branch May 2, 2024 16:12
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

API: argument checking Validation of acceptable argument values status: needs clarification Issues that need more information to resolve. topic: color/colorbar

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL