Skip to content
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

[Bug]: rcParams['legend.loc'] can't use float-tuple like kwarg legend(loc...) #22338

Closed
cphlewis opened this issue Jan 27, 2022 · 5 comments · Fixed by #25839
Closed

[Bug]: rcParams['legend.loc'] can't use float-tuple like kwarg legend(loc...) #22338

cphlewis opened this issue Jan 27, 2022 · 5 comments · Fixed by #25839
Labels
Good first issue Open a pull request against these issues if there are no active ones! topic: rcparams
Milestone

Comments

@cphlewis
Copy link

Bug summary

We can now use a (x,y) argument for plt.legend(loc=(x,y)),
but using the tuple as a value for rcParams['legend.loc'] fails.

Code for reproduction

import matplotlib.pyplot as plt
import matplotlib as mpl
xs, ys = [1,2,3], [2,3,1]

fig, ax = plt.subplots(3)

ax[0].scatter(xs, ys, label='loc-tuple-arg')
ax[0].legend(loc=(1.1, .5)) # works

ax[1].scatter(xs, ys, label='loc-rcparam-tuple')
mpl.rcParams['legend.loc'] = (0.9, .7)
ax[1].legend() # fails w/o documentation

ax[2].scatter(xs, ys, label='loc-rcparam-str(tuple)')
mpl.rcParams['legend.loc'] = '(.8, .3)'
ax[2].legend() # fails with documentation

Actual outcome

Setting rcParams['legend.loc'] to a tuple fails with

AttributeError: 'tuple' object has no attribute 'lower'

setting rcParams['legend.loc'] to the string representation of a tuple fails with a list of supported values.

Expected outcome

Ideally rcParams['legend.loc'] would apply a tuple the same way the kwarg does.

Second best would be a bit more documentation; 1) reporting the list of supported strings for any un-interpretable value, and 2) mentioning in the default matplotlibrc that only a set of strings are supported.

Additional information

I believe the loc kwarg originally only took string values, but that seems to have been at least a major version ago.

Operating system

Ubuntu

Matplotlib Version

3.3.4

Matplotlib Backend

TkAgg

Python version

Python 3.9.7

Jupyter version

No response

Installation

pip

@alexlenail
Copy link

Also having this issue

@tacaswell
Copy link
Member

As a general comment, including the full traceback, not just the final line makes these much easier to read

In [2]: plt.rcParams['legend.loc'] = (0.9, .7)
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
Cell In [2], line 1
----> 1 plt.rcParams['legend.loc'] = (0.9, .7)

File /usr/lib/python3.10/site-packages/matplotlib/__init__.py:649, in RcParams.__setitem__(self, key, val)
    647             return
    648 try:
--> 649     cval = self.validate[key](val)
    650 except ValueError as ve:
    651     raise ValueError(f"Key {key}: {ve}") from None

File /usr/lib/python3.10/site-packages/matplotlib/rcsetup.py:72, in ValidateInStrings.__call__(self, s)
     69     _api.warn_deprecated(
     70         self._deprecated_since, name=name, obj_type="function")
     71 if self.ignorecase:
---> 72     s = s.lower()
     73 if s in self.valid:
     74     return self.valid[s]

AttributeError: 'tuple' object has no attribute 'lower'

The error is coming out of the validators in rcsetup, not the legend call.

I agree there are two things to do here. The first should be done no matter what, but I am a bit wary of the second.

  1. the string validator should fail gracefully if value is not a string an tell you what strings are allowed (easy difficulty)
  2. the validator used for 'legend.loc' could be extended to allow sequences of floats as well as strings. Pro is users expect it. The slight risk of this is that if any other clients who are expecting a string out of this value they will be broken, but I think that is a small risk and can be handled with documentation. (medium difficulty)

They probably should be done in two PRs. The first only requries the changes at a test, the second will require the changes, a test, an API change note, and a bit of lobbying as to why we really should make this change.

@tacaswell tacaswell added the Good first issue Open a pull request against these issues if there are no active ones! label Nov 15, 2022
@tacaswell tacaswell added this to the v3.7.0 milestone Nov 15, 2022
iofall added a commit to iofall/matplotlib that referenced this issue Dec 3, 2022
raphaelquast pushed a commit to raphaelquast/matplotlib that referenced this issue Dec 5, 2022
melissawm pushed a commit to melissawm/matplotlib that referenced this issue Dec 19, 2022
@QuLogic QuLogic modified the milestones: v3.7.0, future releases Jan 25, 2023
@NoyHanan
Copy link
Contributor

NoyHanan commented May 5, 2023

Hi all, is it still open?

@oscargus
Copy link
Contributor

oscargus commented May 5, 2023

Yes, the original issue is still not solved. Be aware that the test added in #24600 should be modified as part of it (as that test is to make sure that a somewhat sensible error message is fed with a tuple).

@NoyHanan
Copy link
Contributor

NoyHanan commented May 5, 2023

Thanks, I'll start working on it.

NoyHanan added a commit to NoyHanan/matplotlib that referenced this issue May 8, 2023
NoyHanan added a commit to NoyHanan/matplotlib that referenced this issue May 8, 2023
NoyHanan added a commit to NoyHanan/matplotlib that referenced this issue May 8, 2023
NoyHanan added a commit to NoyHanan/matplotlib that referenced this issue May 8, 2023
NoyHanan added a commit to NoyHanan/matplotlib that referenced this issue May 9, 2023
NoyHanan added a commit to NoyHanan/matplotlib that referenced this issue Jul 9, 2023
@QuLogic QuLogic modified the milestones: future releases, v3.8.0 Jul 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good first issue Open a pull request against these issues if there are no active ones! topic: rcparams
Projects
None yet
7 participants