FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fixed legend with legend location "best" when legend overlaps shaded area and text by alanlau28 · Pull Request #27469 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Fixed legend with legend location "best" when legend overlaps shaded area and text - #27469

Merged
ksunden merged 1 commit into
matplotlib:mainfrom
alanlau28:fix-legend-loc
Jan 23, 2024
ConversationCommits (1)ChecksFiles changed
Merged

Fixed legend with legend location "best" when legend overlaps shaded area and text#27469
ksunden merged 1 commit into
matplotlib:mainfrom
alanlau28:fix-legend-loc

Conversation

alanlau28 commented Dec 8, 2023

Copy link
Copy Markdown
Contributor

PR summary

This PR fixes bug #27414 and #23323. The calculation of badness in the legend location for 'best' now considers PolyCollection and Text. For PolyCollection, the paths are now converted to lines to check for hit detection when placing a legend. For Text, the bounding box is now checked for overlap.

PR checklist

github-actions Bot reviewed Dec 8, 2023
View reviewed changes

github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. 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.

ksunden reviewed Dec 8, 2023
View reviewed changes
Comment thread lib/matplotlib/legend.py Outdated
Comment on lines +986 to +989
if isinstance(artist, PolyCollection):
paths = artist.get_paths()
for path in paths:
lines.append(artist.get_transform().transform_path(path))

ksunden Dec 8, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I would probably just have this in the if/elif chain rather than inside of one of the cases.

alanlau28 Dec 8, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Moved it here! https://github.com/matplotlib/matplotlib/pull/27469/files#diff-e7a00bb4a353fa623c1d0c1de5c6ca602b7ab38e8906d4842640983133b9196aR981-R984

ksunden commented Dec 8, 2023

Copy link
Copy Markdown
Member

I do believe this should have a behavior change note, while "best" is intentionally 'we take care of it for you' and so I don't necessarily think it can't change, noting when the change occurs is something we should do.

alextanned commented Dec 8, 2023

Copy link
Copy Markdown
Contributor

I do believe this should have a behavior change note, while "best" is intentionally 'we take care of it for you' and so I don't necessarily think it can't change, noting when the change occurs is something we should do.

I agree. How does the newly added note look?

This was linked to issues Dec 8, 2023
[Bug]: Legend overlaps shaded area in fill_between with legend location "best" #27414
Closed
Legend with "loc=best" does not try to avoid text #23323
Closed

QuLogic commented Dec 9, 2023

Copy link
Copy Markdown
Member

Please see the instructions in our contribution guide.

alextanned commented Dec 9, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Hello, could you please provide some guidance on the failed tests? How should I address the PR cleanliness?

rcomer commented Dec 9, 2023

Copy link
Copy Markdown
Member

The PR cleanliness failure is because you replaced a test image twice. We don't want any more images in the commit history than necessary, so you will need to squash the commits down.

To squash, you can do an interactive rebase. I strongly recommend saving a backup copy of your branch first if you have not done this (much) before.

alanlau28 force-pushed the fix-legend-loc branch 2 times, most recently from 685e6f1 to 474bf00 Compare December 9, 2023 20:44
oscargus reviewed Dec 11, 2023
View reviewed changes
Comment thread lib/matplotlib/legend.py Outdated
oscargus approved these changes Dec 15, 2023
View reviewed changes

oscargus left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks good to me!

@alanlau28 if you can reduce the number of commits that would be great (rebase and fixup/squash)! If not, we just have to remember to squash while merging.

QuLogic commented Dec 15, 2023

Copy link
Copy Markdown
Member

Yes, we definitely do not want these "Empty-Commit" commits with nothing in them.

dstansby commented Dec 28, 2023

Copy link
Copy Markdown
Member

This looks good to go - it just needs rebasing on to the current main branch. @alanlau28 would you be able to do that? Let us know if you want any help, or someone else to do it.

Fix legend auto-placement in the presence of text or PolyCollections
87d05ae 
Co-authored-by: alextanned <74106760+alextanned@users.noreply.github.com>
github-actions Bot added Documentation: API files in lib/ and doc/api and removed status: needs rebase labels Jan 21, 2024

rcomer commented Jan 21, 2024

Copy link
Copy Markdown
Member

I think it would be good to get this in, so I took the liberty of rebasing and squashing. The conflict was just because an adjacent branch of the if-loop was modified at #27517.

ksunden added this to the v3.9.0 milestone Jan 23, 2024
ksunden merged commit f976947 into matplotlib:main Jan 23, 2024
rcomer mentioned this pull request Dec 13, 2024
Closed
rcomer mentioned this pull request May 5, 2025
Closed
2 of 6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ksunden ksunden left review comments
github-actions[bot] github-actions[bot] left review comments
dstansby dstansby approved these changes
oscargus oscargus approved these changes

Assignees

No one assigned

Labels

Documentation: API files in lib/ and doc/api topic: legend

Projects

First Time Contributors
Status: Merged

Milestone

v3.9.0

Development

Successfully merging this pull request may close these issues.

[Bug]: Legend overlaps shaded area in fill_between with legend location "best" Legend with "loc=best" does not try to avoid text

8 participants

Back | FazBrowse Home | New Git URL