Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
Add legend support for PatchCollection#30756
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Add legend support for PatchCollection#30756
Uh oh!
There was an error while loading.Please reload this page.
Conversation
The @image_comparison test requires a baseline image that doesn't exist yet.Removing it for now - we still have 4 comprehensive functional tests thatverify the functionality works correctly.
Close figures explicitly at the end of each test to prevent anypotential state leakage that could affect other tests in parallelexecution environments.
timhoffm left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks! Looks good. Only minor style improvements to the tests.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
lib/matplotlib/tests/test_legend.py Outdated
| deftest_patchcollection_legend_match_original(): | ||
| # Test PatchCollection legend with match_original=True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we need this test? The legend patch takes the properties from the first patch.match_original applies the properties from the first patch to all other patches. I don't see how any reasonable implementation ofmatch_original could adversingly affect the legend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I've removed thetest_patchcollection_legend_match_original test. You're absolutely right - since the legend handler always takes properties from the first patch in the collection,match_original doesn't affect the legend behavior. The test was redundant withtest_patchcollection_legend_properties.
Now we have 3 focused tests:
test_patchcollection_legend- Basic functionality (label appears in legend)test_patchcollection_legend_properties- Visual properties are preservedtest_patchcollection_legend_empty- Edge case: empty collection doesn't crash
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
lib/matplotlib/tests/test_legend.py Outdated
| # The legend handle should exist (with default/transparent colors) | ||
| assertlen(leg.legend_handles)==1 | ||
| assertisinstance(leg.legend_handles[0],mpatches.Rectangle) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we want to leave this out? I'd claim that being aRectangle is an implementation detail that we don't want to test.
Instead we should check the color.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I've removed theisinstance(Rectangle) checks from both tests.
Intest_patchcollection_legend, I replaced it with color verification - now checking that the legend patch has the correct facecolor and edgecolor.
Intest_patchcollection_legend_empty, I simply verify that the handle exists since the main goal is to test that empty collections don't crash.
This way the tests focus on actual behavior (correct colors) rather than implementation details (patch type).
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
As noted by@timhoffm, this test doesn't add value because:- match_original affects how colors are applied to patches in the collection- The legend handler always takes properties from the first patch- Whether match_original is True or False, the legend gets the same result- This is already covered by test_patchcollection_legend_properties
As suggested by@timhoffm:- Remove isinstance(Rectangle) checks - this is an implementation detail- In test_patchcollection_legend: Check colors match instead- In test_patchcollection_legend_empty: Just verify legend handle existsThis focuses tests on actual behavior (correct colors) rather thaninternal implementation choices (what patch type is used).
FazeelUsmani commentedNov 19, 2025
Hi@timhoffm, |
rcomer commentedNov 19, 2025
Unfortunately the bot that posts it is currently broken, but when you opened your first PR you should have got a comment withthis message. So about a week is reasonable to wait for a review. |
lib/matplotlib/legend_handler.py Outdated
| # PatchCollection and PolyCollection have the same API for accessing | ||
| # colors, line properties, etc., so we can reuse HandlerPolyCollection | ||
| # implementation directly via inheritance. | ||
| pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What's the reason to bother w/ a new class vs. just directly registering, e.g.:
PatchCollection:legend_handler.HandlerPolyCollection()
FazeelUsmaniNov 20, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There's no reason.PatchCollection andPolyCollection have identical APIs for accessing colors and line properties, so we can just registerHandlerPolyCollection directly. I'll remove the new class.
lib/matplotlib/tests/test_legend.py Outdated
| ax.set_xlim(0,1) | ||
| ax.set_ylim(0,1) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
does this matter if you're just testing object properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No, it doesn't matter. Since we're only testing that the legend handler creates the correct objects with the right properties, the axis limits aren't needed. I'll remove these lines.
lib/matplotlib/tests/test_legend.py Outdated
| # Verify that visual properties are preserved | ||
| legend_patch=leg.legend_handles[0] | ||
| assert_allclose(legend_patch.get_facecolor()[:3], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why is alpha getting ignored here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It shouldn't be. The alpha channel should also be verified. I'll change it to check all 4 RGBA components instead of just RGB.
- Remove HandlerPatchCollection class - use HandlerPolyCollection directly Since PatchCollection and PolyCollection have identical APIs, we can register PatchCollection to use the existing handler without creating a new class. This follows YAGNI principle.- Check all 4 color components (RGBA) instead of just RGB The alpha channel should also be verified for completeness.- Remove unnecessary axis limits from empty collection test Since we're only testing object properties, not visual rendering, the axis limits aren't needed.Addresses feedback from@story645
rcomer commentedNov 20, 2025
You may find thesame_color function useful for the tests. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
lib/matplotlib/tests/test_legend.py Outdated
| pc.get_edgecolor()[0],rtol=1e-5) | ||
| deftest_patchcollection_legend_properties(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You can combinetest_patchcollection_legend: andtest_patchcollection_legend_properties(): b/c text is just another property
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ok
story645 left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for the contribution! test failures seem unrelated but I'm gonna let them run again to make sure.
7a7a388 intomatplotlib:mainUh oh!
There was an error while loading.Please reload this page.
story645 commentedNov 26, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
|
FazeelUsmani commentedNov 27, 2025
@story645 You're welcome! Haha, yes, I want to adopt AI in every aspect of my day-to-day life. |
Uh oh!
There was an error while loading.Please reload this page.
PR summary
Closes#23998
Why is this change necessary?
PatchCollectioninstances do not currently display in legends even when assigned a label. This has been a long-standing limitation that forces users to create manual legend entries, making the API inconsistent with other collection types likePolyCollectionandPathCollection.What problem does it solve?
Before this fix, the following code would not display a legend entry:
After this fix, the legend correctly displays the "my patches" label with a rectangular swatch matching the collection's visual properties.
Implementation reasoning
The implementation adds a new
HandlerPatchCollectionclass that inherits from the existingHandlerPolyCollection. This approach:PatchCollectionandPolyCollectioninherit fromCollectionand share identical APIs for accessing colors, line styles, and other visual propertiesPolyCollectionHandlerPolyCollectionautomatically benefitHandlerPatchCollectionThe handler extracts visual properties (face color, edge color, line width, line style) from the first patch in the collection and creates a rectangular legend entry.
Minimum self-contained example
PR checklist
match_original=Trueparameter@image_comparisondoc/release/next_whats_new/patchcollection_legend.rstHandlerPatchCollectionincludes comprehensive docstring