Bugfix for loc legend validation #25281

robrighter · 2023-02-22T05:06:30Z

Co-authored-by: John Paul Jepko jpjepko@users.noreply.github.com

PR Summary

This pull request adds a check to the loc argument of legend. It checks that loc is one of the following:

valid location string (this validation was already present)
a 2-tuple of floats or ints
a string numeric value (0-10)

The code raises a ValueError otherwise. This resolves #24605 . The docstrings already mentioned the fact that loc is a pair of floats so we did not add any changes to any docstrings. We also added test cases for the new validation checks. For details on the valid options refer to this doc

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.

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

rcomer · 2023-02-22T07:53:36Z

@robrighter in your original post, if you replace “addresses the issue #24605” with just “resolves #24605”, it will link the issue so it automatically closes when this is merged. You could also link to the old PR in the same way so that closes too. There are various keywords that work:
https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

robrighter · 2023-02-22T13:16:01Z

Thanks @rcomer , I updated the initial comment with "resolves #24605"

tacaswell · 2023-02-22T17:47:06Z

lib/matplotlib/legend.py

+            if len(loc) != 2 or not (all(isinstance(e, numbers.Real)
+                                         for e in loc)):
+                raise ValueError(
+                    f"loc must be string or pair of numbers, not {loc!r}")


Should this include a note about [0, 9] being allowed?

Yes, but I'm unsure about the wording. Maybe "loc must be string, coordinate tuple or, an integer 0-10, not xyz"

for bonus points you can also include the list of allowed strings!

One of the goals of our error messages should be, if it makes sense, to give the user enough of a hint about what they did wrong so that they can sort out how to fix it just from reading the traceback (and not have to go open a browser to search for the docs).

This thought turned into #25292

I updated the pull request to be more verbose. In your error message guidelines doc you mentioned that we should try to use the helper methods to standardize the error messages. I think that is good advice but it limits the ability to customize the error message to fit the context. To strike the balance in this situation, my suggestion is:

Use a less verbose error message when the user provides a non-string type such as " "loc must be string, coordinate tuple or, an integer 0-10, not 21"

Use the built in (and more verbose) error message when the user attempts to provide a string that does not match the allowed strings. This uses _api.check_getitem and yields a message like: "'xyz' is not a valid value for loc; supported values are 'best', 'upper right', 'upper left', 'lower left', 'lower right', 'right', 'center left', 'center right', 'lower center', 'upper center', 'center'"

@tacaswell let me know if this tradeoff works for you or if you think I should update all the responses to include all the information. Either works for me. ^

robrighter · 2023-03-02T00:00:59Z

@tacaswell are there any additional changes you would like to see on this PR?

lib/matplotlib/legend.py

lib/matplotlib/tests/test_legend.py

lib/matplotlib/legend.py

lib/matplotlib/tests/test_legend.py

rcomer · 2023-03-11T11:34:58Z

lib/matplotlib/legend.py

@@ -535,6 +539,17 @@ def val_or_rc(val, rc_name):
                    loc = locs[0] + ' ' + locs[1]
            # check that loc is in acceptable strings
            loc = _api.check_getitem(self.codes, loc=loc)
+        elif isinstance(loc, tuple):


I'm wondering if we should be more lenient here: currently there is nothing to stop a user passing e.g. a 2-element list, and it works just fine, but this will presumably break anyone doing so. Not long ago we had a similar discussion about the iterable passed to colorbar, and ultimately decided to be more lenient than had previously been documented #24408.

This seems reasonable to me. If we allow a list we should:

check the length and error on the wrong length.

update the docs and make it a supported type.

What do you think?

For maximum flexibility, we could attempt to convert anything that isn't str or int into a tuple within a try-except loop. Then just use your existing handling for tuple. Probably better to wait and see what other core developers think though.

I went ahead an added the case for lists and then convert the list into a tuple. The tuple logic then evaluates the length, etc.

I changed my mind on the documentation. It seems like we are only allowing lists for backward compatibility. Tuples are probably the better choice and so the api should probably advertise tuples as the valid type for coordinate pairs.

lib/matplotlib/tests/test_legend.py

rcomer · 2023-03-13T11:34:13Z

lib/matplotlib/legend.py

        # handle outside legends:
        self._outside_loc = None
+
+        # coerce list into tuple for backward compatibility
+        if isinstance(loc, list):


Suggested change

if isinstance(loc, list):

if isinstance(loc, collections.abc.Iterable):

This would make it more general. A list is the most obvious type of iterable someone might pass, but there may be others. Your decision on the documentation makes sense to me.

We seem to rely on np.iterable for this type of checks.

(It should probably be collections.abc.Iterable as well as importing directly is removed in 3.11.)

Oh wait, we don’t want to convert all iterables here, because that would include strings and we clearly don’t want them to become tuples! So I think this should come after the string handling…

rcomer · 2023-03-13T12:14:06Z

lib/matplotlib/legend.py

        # handle outside legends:
        self._outside_loc = None
+
+        # coerce list into tuple for backward compatibility
+        if isinstance(loc, list):


Oh wait, we don’t want to convert all iterables here, because that would include strings and we clearly don’t want them to become tuples! So I think this should come after the string handling…

rcomer · 2023-03-13T12:15:56Z

lib/matplotlib/legend.py

@@ -535,6 +544,17 @@ def val_or_rc(val, rc_name):
                    loc = locs[0] + ' ' + locs[1]
            # check that loc is in acceptable strings
            loc = _api.check_getitem(self.codes, loc=loc)
+        elif isinstance(loc, tuple):


Suggested change

elif isinstance(loc, tuple):

elif np.iterable(loc)

loc = tuple(loc)

lib/matplotlib/legend.py

Co-authored-by: John Paul Jepko <jpjepko@users.noreply.github.com>

rcomer · 2023-03-16T07:45:38Z

Thanks @robrighter!

robrighter marked this pull request as ready for review February 22, 2023 05:22

tacaswell added this to the v3.8.0 milestone Feb 22, 2023

tacaswell reviewed Feb 22, 2023

View reviewed changes

robrighter force-pushed the bugfix-for-loc-legend-validation branch 3 times, most recently from 385387b to 5a30ae7 Compare February 23, 2023 14:07

jklymak requested a review from tacaswell February 24, 2023 19:42

robrighter changed the title ~~RR - bugfix for loc legend validation~~ Bugfix for loc legend validation Feb 25, 2023

rcomer reviewed Mar 5, 2023

View reviewed changes

lib/matplotlib/legend.py Outdated Show resolved Hide resolved

lib/matplotlib/tests/test_legend.py Outdated Show resolved Hide resolved

robrighter force-pushed the bugfix-for-loc-legend-validation branch 3 times, most recently from 6d3027d to 51385ed Compare March 7, 2023 03:47

QuLogic reviewed Mar 7, 2023

View reviewed changes

lib/matplotlib/legend.py Outdated Show resolved Hide resolved

lib/matplotlib/legend.py Outdated Show resolved Hide resolved

lib/matplotlib/legend.py Show resolved Hide resolved

lib/matplotlib/tests/test_legend.py Outdated Show resolved Hide resolved

rcomer mentioned this pull request Mar 7, 2023

Fix loc legend validation #24649

Closed

6 tasks

robrighter force-pushed the bugfix-for-loc-legend-validation branch 2 times, most recently from 23f5667 to 0634c0a Compare March 7, 2023 23:14

adityasingh22 mentioned this pull request Mar 10, 2023

[Bug]: Validation not performed for loc argument to legend #24605

Closed

QuLogic approved these changes Mar 11, 2023

View reviewed changes

rcomer reviewed Mar 11, 2023

View reviewed changes

robrighter force-pushed the bugfix-for-loc-legend-validation branch 2 times, most recently from 1684278 to 379fc6c Compare March 13, 2023 00:39

rcomer reviewed Mar 13, 2023

View reviewed changes

robrighter force-pushed the bugfix-for-loc-legend-validation branch from 379fc6c to 586e585 Compare March 15, 2023 00:38

rcomer reviewed Mar 15, 2023

View reviewed changes

lib/matplotlib/legend.py Outdated Show resolved Hide resolved

Bugfix for loc legend validation

f263f7d

Co-authored-by: John Paul Jepko <jpjepko@users.noreply.github.com>

robrighter force-pushed the bugfix-for-loc-legend-validation branch from 586e585 to f263f7d Compare March 15, 2023 23:42

rcomer approved these changes Mar 16, 2023

View reviewed changes

rcomer merged commit 63b9b04 into matplotlib:main Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix for loc legend validation #25281

Bugfix for loc legend validation #25281

robrighter commented Feb 22, 2023 •

edited

Loading

rcomer commented Feb 22, 2023

robrighter commented Feb 22, 2023

tacaswell Feb 22, 2023

robrighter Feb 22, 2023

tacaswell Feb 22, 2023

tacaswell Feb 22, 2023

robrighter Feb 23, 2023

robrighter Feb 23, 2023

robrighter commented Mar 2, 2023

rcomer Mar 11, 2023

robrighter Mar 11, 2023

rcomer Mar 11, 2023 •

edited

Loading

robrighter Mar 13, 2023

rcomer Mar 13, 2023 •

edited

Loading

oscargus Mar 13, 2023

rcomer Mar 13, 2023

robrighter Mar 15, 2023

rcomer Mar 13, 2023

rcomer Mar 13, 2023

robrighter Mar 15, 2023

rcomer commented Mar 16, 2023

	if isinstance(loc, list):
	if isinstance(loc, collections.abc.Iterable):

	elif isinstance(loc, tuple):
	elif np.iterable(loc)
	loc = tuple(loc)

Bugfix for loc legend validation #25281

Bugfix for loc legend validation #25281

Conversation

robrighter commented Feb 22, 2023 • edited Loading

PR Summary

PR Checklist

rcomer commented Feb 22, 2023

robrighter commented Feb 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robrighter commented Mar 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcomer Mar 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcomer Mar 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcomer commented Mar 16, 2023

robrighter commented Feb 22, 2023 •

edited

Loading

rcomer Mar 11, 2023 •

edited

Loading

rcomer Mar 13, 2023 •

edited

Loading