Setting color of legend shadow

[tuncbkose]

PR Summary

Allows shadow legend parameter to be a color.
Start making legend.shadow configurable.
As requested by #24663, revival of #18668, based on previous work by @Tranquilled.

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).
• [?] New plotting related features are documented with examples.
  • The change is relatively small, but I can add examples if requested.

Release Notes

• New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
• API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
• Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

[github-actions]

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

[tacaswell]

I'm a little wary of going to strict type checking, there is a strong expectations that 0 and 1 will work basically anyplace True / False will.

[tuncbkose]

I am under the impression that 0 and 1 are all one might expect to work, as opposed to options like "true", "false" as in validate.bool in rcsetup.py.
Do you think it is sufficient to change isinstance to shadow in (0, 1, True, False)?

[timhoffm]

@tacaswell I'm somewhat annoyed by the 0, 1 topic.

In many cases (also here before the PR), we do implicit bool-casting via if shadow:. That means users can pass 0 or [] or whatever they like. I don't think we give a guarantee that these will work in the future.

In my personal opinion using 0, 1 as a bool substitute is an anti-pattern that reduces readability. (e.g. here it could also be the size of the shadow). While I don't want to dictate how people are using python. I don't think we need to jump extra hoops to support unreasonable behavior. Anyway, what I definitively don't want is cluttering our code internally with 0, 1 checks.

We should

• either stay as permissive as before (i.e. do not do any check here), which is somewhat problematic as mistyped color strings are interpreted as True
• or do the strict type checking

If you insist that 0, 1 is really worth supporting, we should have a general discussion on that in the dev call. We'd need to document what exactly we want to accept in such cases in the dev guidelines. And we should come up with supporting functionality like is_bool_like().

On a more general note:

Are we sure that the shadow=[color] extension is the right API? It is ok if we are quite sure there won't be additional parameters in the future. However, if we think there will be other parameters like size or blur, we should go for another API. One way would be to bool or dict so that you can do shadow={'color': 'red', 'size'=5}, which is probably better than introducing additional parameters.

[anntzer]

I would suggest the correct way to check for T/F is perhaps something like np.ndim(x) == 0 and (self.shadow == False or self.shadow == True)? (where the np.ndim check is only to avoid problems with array-equality returning bool arrays that cannot be used in a bool scalar context). This way you take advantage of the fact that True == 1 and False == 0 (a fact that also makes supporting T/F but not 0/1 a bit weird; note that the same argument does not apply to other truthy or falsy values).

[oscargus]

Failure seems unrelated.

I am wondering if one should replace one of the legends (or possibly add one in the middle) that uses the rcParams?

Regarding the 0/1 issue, I'll add that to the agenda for the dev call tomorrow.

[oscargus]

Oh, and if you know how to rebase and squash, please do that.

It would also make sense to edit the commit message and add the line (after two empty lines):

Co-authored-by: Tranquilled (or proper name)  <whatever the email is>

[tuncbkose]

I don't know how to rebase and squash, but I can probably figure it out. And add the coauthor message too.

Are there any updates on the 0/1 issue that warrants changing the code here?

[rcomer]

@tuncbkose we have instructions for squashing here:
https://matplotlib.org/devdocs/devel/development_workflow.html#rewriting-commit-history

[tacaswell]

Thank you for your work on this @tuncbkose however I'm going to push this to 3.8. There are a couple of outstanding questions that will take too long to sort out to sneak this in for 3.7.

• Should we make the input bool or dict as Shadow will take all of the Patch keyword arguments. As @timhoffm points out if we start making it possible to thread the color through it is likely there will soon be a demand for threading through the rest of the parameters. This then leads to the question of how to handle the rcparams as we do not currently have any structured values (there is no validate_dict) so either we would have to add that (which would be a lot of work) or accept that there are values you can pass at run time that can not be set in rcparams. Neither are great options.
• Another option would be to add a shadow_props kwarg. The upside of this is we side-step all of the rcparams issues (at least for a bit), but the major down side is now we have coupled kwargs (where the value of one kwarg affects if we even pay attention to the other). It would not be the first place we have this, but they are messy to document and use (but sometimes you need them for "Do What I Mean").
• @timhoffm and I need to sort out what to do about the {0, 1} cases (this is the second or third one to come up recently). On one hand I agree with Tim that the using 0 and 1 literals is place of True and False is not good, but on the other hand I am still very wary of ripping out the duck-typing (but not ideas I would endorse) with no warning.

While this seems like a very small change (and the actually implementation is straight forward), there are a bunch of follow-on consequences. Some of the more gnarly parts of the Matplotlib API are due to adding features that were relatively small but eventually began to interact in surprising ways!

I think my favorite path forward is:

• make the shadow kwarg to be None | bool | dict
• if setting the color via rcparams is a hard requirement add at legend.shadow_props.color entry (so we have space to add more if needed)

I think something like:

if shadow is None:
    self.shadow = rcparams['legend.shadow']
    self._shadow_props = {}
elif instance(shadow, dict):
    self._shadow_props = shadow
    self.shadow = bool(shadow)
else:
    self.shadow = bool(shadow)
    self._shadow_props = {}

# maybe
for k in ['color']:
    self._shadow_props.setdefault(k, rcparams[f'legend.shadow_props.{k}'])

...

if self.shadow:
    Shadow(self.legendPatch, 2, -2, **self._shadow_props).draw(renderer)

This:

• side-steps the "how much duck type preservation" by preserving the existing behavior at the cost of deciding the ambiguity of shadow={} should mean in terms of "bool like" not "props like"
• lets the user control the shadow color at the call sight
• also takes care of any future "but what about the " issues now
• gives an (optional) path to setting a subset of the properties via rcparams independently of setting if the shadow is shown by default or not

The biggest down side is a further growth of the number of rcparams, but I think that is marginally less bad than the existing rcparams getting more complex.

[tacaswell]

My last comment is the summary of a phone discussion with @QuLogic and @ksunden .

[tacaswell]

see long comment above.

Anyone can dismiss this review.

[jklymak]

Moving this to draft, and marking inactive, however, @tuncbkose please let us know if you are still interested in this and we can move back to the queue...

[tuncbkose]

I am happy to implement whatever that is decided to be preferable. It was (and still is) unclear to me if @tacaswell's last comments were only a suggestion or the agreed way to move forward. If it's the latter, I can probably make the necessary changes soon.

[timhoffm]

I think my favorite path forward is:

• make the shadow kwarg to be None | bool | dict

When we allow for this type variability in parameter values (which is quite reasonable), IMHO we can as well afford an intermediate color step: None | bool | color | dict. This allows us to do the color implementation now. The dict implementation can be left for later when the actual need for further configuration arises. But we know it's there as an extension path without the need to grow additional or conflicting further kwargs.

if setting the color via rcparams is a hard requirement add at legend.shadow_props.color entry (so we have space to add more if needed)

Is there some proven interest in rcParams availability? If not, let's start simple and leave this out - we have a number of setting that are not available through rcParams.

Rationale: If we start shadow configuration, this has to go into a new 'shadow_props' section immediately. As opposed to the input in parameters, rcParams can be read. While we could overload this with color (every color is truthy, so current (duck-typed) bool checks would continue to work), we cannot transparently change this to a dict. It would also be unwise to have rcParams['shadow'] == *a-color* and additionally start a rcParams['shadow_props'], due to inconsistent/duplicate handling of values.

I'm also considering a maybe too clever alternative: Can we turn the existing 'legend.shadow' into a namespace directly, i.e. 'legend.shadow.color' etc. while staying backward-compatible.

a) Parsing an rcParams file should be simple: promote 'legend.shadow' to 'legend.shadow.visible'
b) Code usage: We still need API compatibility. This would mean that rcParams would need to grow special getitem/setitem logic for 'legend.shadow' (at least for a transition period).

This is quite a bit of effort, but would allow a more canonical rcParams namespacing in the future. I think, we can pull this of, but it's only worth if there is really the need. Hence, the proposal to not touch rcParams for now (well, we have to explicitly raise on colors now; otherwise users can think colors are supported, but we actually duck-type them as bool).

Can we have a similar generalization strategy for rcParams?

• For now use legend.shadow valued None | bool | color
• If there is need for more, introduce legend.shadow_props

legend.shadow.visible
legend.shadow.color

[anntzer]

Just from the peanut gallery...

This then leads to the question of how to handle the rcparams as we do not currently have any structured values (there is no validate_dict) so either we would have to add that (which would be a lot of work)

I do think that making some rcParams be dicts (with a specified key/value model) would be good. (See also #11231.)

[tuncbkose]

Here is a first implementation of shadow as None|bool|dict without rcParam support (as a mixture of suggestions by @timhoffm and @tacaswell). Currently, the dict contents are not checked because I assume Patch will do that already. If requested I can re-add shadow=[color] as well, but in my opinion having to do {"color": [color]} is only slightly different without the risk of potentially deprecating it later.

One question I have in addition to general review: should rcParams give a specific error message saying dict is not currently supported?

[rcomer]

Thanks for the update @tuncbkose. If you look at the code diff you can see there are some comments from bots. The flake8 comments need to be addressed, and should be straightforward. The CodeCov comments may or may not be sensible, but it’s worth considering whether the second one indicates a genuine gap in the tests.

[tuncbkose]

I'm aware of the current formating/styling issues. I just didn't fix them yet, in case more changes are requested. If it would be more convenient for the reviewers, I'll happily fix the failing issues now.

[melissawm]

Gentle ping @tuncbkose - please let us know if you need help addressing the reviews above. Cheers!

[tuncbkose]

Thanks for the ping @melissawm. Sorry that I forgot about this, I should be much more attentive now, if anything further is needed.

[tuncbkose]

These errors seem unrelated to me.

[timhoffm]

Minor wording suggestions.

Comment addressed.

[ksunden]

One really small change is that the type hint should be updated to reflect dict being allowed.

(Most changes that require such cause CI failures, but this is a particular case that does not)

matplotlib/lib/matplotlib/legend.pyi

Line 81 in adc8c76

shadow: bool | None = ...,

Should include, likely, | dict[str, Any] (if all possible values are enumerable and manageable, can be more specific than Any, but tend towards readability.

Arguably the attribute form could have it as well, but the practical implementation is it only ever gets set to a dictionary during __init__, and is made to be a bool before leaving such, so lean towards not changing that.

And one question that I would readily accept the answer of "no" or "not yet" and just be done:

Do we want a set_shadow method to allow editing of the shadow properties post-hoc? currently the self.shadow is a bool and self._shadow_props is a private attribute, so no public way to update it, as far as I can tell.

[timhoffm]

Do we want a set_shadow method to allow editing of the shadow properties post-hoc? currently the self.shadow is a bool and self._shadow_props is a private attribute, so no public way to update it, as far as I can tell.

I go with „not yet“. The signature is not quite obvious to me. Is it a single (dict | bool) parameter (possibly, but a bit awkward for set_shadow({„color“: „red“})) or can we unpack into separate args so that set_shadow(True) andset_shadow(color=„red“) - works? The latter may be difficult to write a clean signature for. Or we could expose the underlying artist for styling?

Overall, let‘s wait with this until a request for this is raised.

[oscargus]

@ksunden it seems like your requested change is done.

requested changes addressed

[ksunden]

Test failures appear to be due to qt problems unrelated to this PR, but rerunning just to be sure.

[melissawm]

Congrats @tuncbkose on getting your first PR merged into Matplotlib - we hope to see you around for a second one! 😄