Skip to content

replaced _check_color_like method with call to to_rgba #25140

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

story645
Copy link
Member

@story645 story645 commented Feb 3, 2023

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

rcomer commented Feb 3, 2023

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

story645 commented Feb 3, 2023

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 jklymak added the status: needs clarification Issues that need more information to resolve. label Feb 24, 2023
@jklymak
Copy link
Member

jklymak commented Feb 24, 2023

Can we clarify the status of this one?

@jklymak jklymak marked this pull request as draft June 14, 2023 23:43
@jklymak
Copy link
Member

jklymak commented Jun 14, 2023

I've moved to draft.

@story645
Copy link
Member Author

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

@story645 story645 closed this Jun 23, 2023
@story645 story645 deleted the color-check branch May 2, 2024 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants