Fix loc legend validation #24649
Fix loc legend validation #24649
Conversation
There was a problem hiding this comment.
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.
|
It looks like what happened is when you tried to push it failed, the message suggested you merge, you did so, and then the push worked? What is going on is that by default git's push tries to keep you from accidentally make commits inaccessible so if a push would orphan commits git fails and github helpfully suggest you merge the remote work with your and then push (both of which are good ideas). However, in this case it is our goal to re-write history and we want to make some commits disappear and replace them with new and improved commits so we need to tell git that we really mean it. The incantation you want here is which will tell git to do the push anyway, so long as you have at least had a chance to see the commits that you are going to be making unavailable (there is also |
|
(just edited because there was a 1 character typo in there leave -> lease) |
|
I believe I had forgotten to point the upstream to the matplotlib repo Could that have been the culprit? In any case, would you prefer for me to open a new PR to keep the commit history clean? |
|
There's no need for that; rebase and drop the extra commits, then follow @tacaswell's instructions for force pushing the new ones. |
03813be to
a7e867c
Compare
|
Thank you for your guidance. I cleaned up the commit history, rebased, and force pushed as per the updated Development workflow in the dev docs. Are there any other changes you would like for me to incorporate? |
a7e867c to
aacac4c
Compare
|
Hey @QuLogic, is there anything else that might need any changes? Thank you for your continued guidance. |
lib/matplotlib/legend.py
Outdated
| if len(loc) != 2: | ||
| raise ValueError( | ||
| "loc must be string or pair of floats") | ||
| for loc_elt in loc: | ||
| # int can be converted to float, so ints are fine | ||
| if not isinstance(loc_elt, (float, int)): | ||
| raise ValueError(f"{loc_elt} is not a valid value for loc") |
There was a problem hiding this comment.
| if len(loc) != 2: | |
| raise ValueError( | |
| "loc must be string or pair of floats") | |
| for loc_elt in loc: | |
| # int can be converted to float, so ints are fine | |
| if not isinstance(loc_elt, (float, int)): | |
| raise ValueError(f"{loc_elt} is not a valid value for loc") | |
| if len(loc) != 2 or not all(isinstance(e, Real) for e in loc): | |
| raise ValueError( | |
| "loc must be string or pair of numbers, not {loc!r}") | |
| else: | |
| raise ValueError( | |
| "loc must be string or pair of numbers, not {loc!r}") |
- simplify to check for
NumberReal - merge the len and the number checks
- error out for other input types
edit: modified to include suggestions by @QuLogic and @tacaswell
There was a problem hiding this comment.
I think we want numbers.Real, as in:
matplotlib/lib/matplotlib/axes/_secondary_axes.py
Lines 95 to 96 in cc85fb6
There was a problem hiding this comment.
Just incorporated these changes in the original commit. Thanks!
|
Sorry to be yet another person chiming in from the 🥜 gallery, but can you also add the repr of what the user did pass to the error message? |
|
I'm going to milestone this as 3.8, if it is ready and merged it can definitely go in for 3.7, but I am trying to whittle down the milestone to things we would hold 3.8 over in the next 2 weeks! |
aacac4c to
5b5d17d
Compare
5b5d17d to
0b7fcba
Compare
Oops, just realized that tests not passing are the result of this change.
|
Pushed back to 3.8 as there are failing tests still. |
|
@jpjepko are you hoping to come back to this? Let us know if you need anything, but it looks like you are missing some valid loc values |
|
Closing in favour of #25281. |
PR Summary
This pull request adds a check to the
locargument oflegend. It checks thatlocis a 2-tuple of floats or ints, and raises aValueErrorotherwise. This addresses the issue #24605. The docstrings already mentioned the fact thatlocis a pair of floats so we did not add any changes to any docstrings. We also added test cases for valid and invalid tuples.PR Checklist
Documentation and Tests
pytestpasses)Release Notes
.. versionadded::directive in the docstring and documented indoc/users/next_whats_new/.. versionchanged::directive in the docstring and documented indoc/api/next_api_changes/next_whats_new/README.rstornext_api_changes/README.rst