Prevent 'color' argument to eventplot from overriding 'colors' kwarg (fixes #4297) #4298
Conversation
|
Seems more-or-less reasonable to me. I am inclined to raise, or at least warn, if the user passes in conflicting input. attn @efiring |
|
It looks like colors/color is not the only potential conflict here; maybe they can all be cleared up in one shot. I'm also wondering whether there might be a simple and more general way to handle this sort of thing, which seems to be a symptom of our loose handling of **kwargs. Maybe that should be considered separately. For the present PR, it might be enough to just loop over the pairs of potentially conflicting kwargs so as to handle all of them in a single simple loop. |
| _color = kwargs.pop('color', None) | ||
| # 'colors' kwarg should have a higher priority than 'color' | ||
| if colors is None: | ||
| colors = _color |
There was a problem hiding this comment.
Can you add an
else:
if _color is not None:
warnings.warn("'color' kwarg ignored")or something to that effect?
|
@efiring I am fine with merging this (modulo the warning comment) now. The more general clean up should be done library wide and is a much bigger task than we want to pin on a new contributor. |
alimuldal
force-pushed
the
color_prop_override
branch
from
6b36357
to
7246263
Compare
|
Apologies for not following up on this sooner (I've been very busy this week). Following Eric's suggestion, I've added a function to |
alimuldal
force-pushed
the
color_prop_override
branch
from
7246263
to
3f7d083
Compare
| # remove 'singular' keys from **kwargs dict in order to prevent them | ||
| # being lazily passed as LineCollection props, where they override the | ||
| # effect of the 'plural' versions (e.g. 'color' overrides 'colors') | ||
| cbook.strip_problem_kwargs(kwargs, 'color', 'linewidth', 'linestyle') |
There was a problem hiding this comment.
This just discards the 'bad' kwargs which we can't do without a deprecation cycle. There are users out there that have been using color that this will break. This needs to automate the precedence cascade.
There was a problem hiding this comment.
Yes, I thought that was your intent, based on your suggestion to add a warning to the effect of warnings.warn("'color' kwarg ignored")
There was a problem hiding this comment.
I wanted that warning only if the kwarg was actually ignored.
alimuldal
force-pushed
the
color_prop_override
branch
from
3f7d083
to
4a0eb3e
Compare
|
@tacaswell I think this now gives the behaviour you are looking for |
| Enforces the priority of a local variable over a potentially conflicting | ||
| argument in a kwargs dict, but falls back on the value of the keyword | ||
| argument if the local variable's value is None. Raises an | ||
| IgnoredKeywordWarning if the keyword argument is specified but ignored. |
There was a problem hiding this comment.
Can you please add numpydoc style Paramaters and Returns sections to this docstring?
|
Yes, thank you. Just a minor docstring change left (I hope), other than that lgtm to me. attn @efiring |
alimuldal
force-pushed
the
color_prop_override
branch
from
4a0eb3e
to
57f67e6
Compare
| --------- | ||
| out: any object | ||
| If local_var is None then kwargs[kwarg_name] will be returned, | ||
| otherwise local_var will be returned and an IgnoredKeywordWarning |
There was a problem hiding this comment.
This is not correct. -> "otherwise local_var will be returned. If kwargs[kwarg_name] has been overridden by local_var, an IgnoredKeywordWarning will be raised." Actually, I think the sentence about the warning should be in a separate "Raises:" block, shouldn't it?
alimuldal
force-pushed
the
color_prop_override
branch
from
57f67e6
to
4683061
Compare
|
@efiring I've written a more general version that can parse several keyword arguments in descending order of priority. The name, signature and docstring have also been changed according to your suggestions. |
alimuldal
force-pushed
the
color_prop_override
branch
5 times, most recently
from
b59bf4a
to
980e551
Compare
alimuldal
force-pushed
the
color_prop_override
branch
from
980e551
to
8b083c1
Compare
|
@tacaswell @efiring Are you happy with the new version? |
|
Looks good to me. |
Prevent 'color' argument to eventplot from overriding 'colors' kwarg (fixes #4297)
|
@alimuldal Thank you for the work and your patience working with us on revisions! |
Fixes issue #4297