Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Created handler for PatchCollection#24028

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

Open
FarukFS wants to merge1 commit intomatplotlib:main
base:main
Choose a base branch
Loading
fromFarukFS:my-feature

Conversation

FarukFS
Copy link

@FarukFSFarukFS commentedSep 28, 2022
edited
Loading

PR Summary

As indicated in Issue#23998, PatchCollection is not supported in legends, even though LineCollection and Patch are supported. It was mentioned that this could be a nice feature to include. Hence, I created a legend handler for PatchCollection objetcts. I must mention that for the color of the legend handler, I don't know whether to use the edgecolor or facecolor, I opted for the last mentioned.

This is my first contribution to an open source project, so I am a bit confused about the documentation. Should this feature have a .rst in doc/users/next_whats_new/ ?

PR Checklist

Tests and Styling

  • Has pytest style unit tests (andpytest passes).
  • IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).

Copy link

@github-actionsgithub-actionsbot left a comment

Choose a reason for hiding this comment

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

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 uson gitter for real-time discussion.

For details on testing, writing docs, and our review process, please seethe developer guide

We strive to be a welcoming and open project. Please follow ourCode of Conduct.

@oscargus
Copy link
Member

Thanks for your contribution!

I think it will be beneficial to have a What's new entry for sure.

What is required though is a test for this. I am not sure how it should work exactly, but at least adding a PatchCollection to an Axes, adding a legend and make sure that there is an entry in the legend is a start. Probably one should also check that the marker/patch is correct as well as the label.

The test can either go inhttps://github.com/matplotlib/matplotlib/blob/main/lib/matplotlib/tests/test_collections.py orhttps://github.com/matplotlib/matplotlib/blob/main/lib/matplotlib/tests/test_legend.py (not sure which is correct, one can argue for both).

FarukFS reacted with thumbs up emoji

def _default_update_prop(self, legend_handle, orig_handle):
lw = orig_handle.get_linewidths()[0]
dashes = orig_handle._us_linestyles[0]
color = orig_handle.get_facecolor()[0]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
color=orig_handle.get_facecolor()[0]
facecolor=orig_handle.get_facecolor()[0]
edgecolor=orig_handle.get_edgecolor()[0]

You need to handle both.

story645 and FarukFS reacted with thumbs up emoji
Copy link
Author

Choose a reason for hiding this comment

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

Thanks! I will do that.

@timhoffm
Copy link
Member

timhoffm commentedSep 28, 2022
edited
Loading

Hint 1: Here is the description how to add a what's new entry:https://github.com/matplotlib/matplotlib/tree/main/doc/users/next_whats_new

Hint 2: This is about the test code we need:

import matplotlib.pyplot as pltfrom matplotlib.collections import PatchCollectionfrom matplotlib.patches import Circle, Rectangleimport matplotlib.colors as mcolorsfig, ax = plt.subplots()pc = PatchCollection([        Circle((0, 0), radius=1, facecolor='red', edgecolor='green', linewidth=3, linestyle='--'),        Rectangle((0.5, 0.5), 1, 1),    ],    match_original=True,    label='my collection')ax.add_collection(pc)handles, labels = ax.get_legend_handles_labels()assert len(labels) == 1assert labels[0] == 'my_collection'assert mcolors.same_color(handles[0].get_facecolor(), 'red')assert mcolors.same_color(handles[0].get_edgecolor(), 'green')assert handles[0].get_linewidth() == 3assert handles[0].get_linestyle() == '--'
FarukFS reacted with thumbs up emoji

@FarukFS
Copy link
Author

Thank you for your help and tips@oscargus@timhoffm

I will try to finish this today!

@FarukFS
Copy link
Author

FarukFS commentedSep 29, 2022
edited
Loading

Edit: I will handle the linting failures and push again.

Hi,

I have pushed some new changes. Now, both facecolors and edgecolors are handled. I also added a what's_new document, as well as a test in test_legend.py. However, I have a few problems/questions.

1.) All the tests are successful, with the exception of the linestyle. Even though the linestyle is what I expected (visibly), the assertion fails. This is because for some reason, the output of "handles[0].get_linestyle()[0]" is (0.0, [11.100000000000001, 4.800000000000001]). Hence, when compared to "--" (which is supposed to be equivalent to (0, (5, 5))) if I'm not wrong), the assertion fails. (This is illustrated in the attached picture)

2.) Is it possible to change the shape of the object inside the legend box? Even though the first object in the collection is a circle, it appears as a rectangle in the legend box, which is clearly not ideal (also shown in the attached picture). Is there an attribute that controls this? Is there any get_xxx method that could help me here?

Assertion error linestyle

@rcomer
Copy link
Member

The dash pattern is converted to numbers here

elifstylein ['dashed','dashdot','dotted']:
offset=0
dashes=tuple(mpl.rcParams['lines.{}_pattern'.format(style)])

So it’s configurable, but assuming it matches thelines.dashed_pattern shown in thedefault matplotlibrc would give (0, (3.7, 1.6)).

Then there is also some scaling where that pattern is multiplied by the linewidth, which is obviously 3 in your case.

def_scale_dashes(offset,dashes,lw):
ifnotmpl.rcParams['lines.scale_dashes']:
returnoffset,dashes
scaled_offset=offset*lw
scaled_dashes= ([x*lwifxisnotNoneelseNoneforxindashes]
ifdashesisnotNoneelseNone)
returnscaled_offset,scaled_dashes

So (11.1, 4.8) seems right to me.

FarukFS reacted with thumbs up emoji


PatchCollection objects are now supported in legends. The feature can be used as follows:

.. code-block:: python
Copy link
Member

Choose a reason for hiding this comment

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

I think you need these directives to make the plot show up.

.. plot::    :include-source: true

Seehere for an example.

FarukFS reacted with thumbs up emoji
@oscargus
Copy link
Member

I'd say that a PatchCollection would be identified with color rather than shape, so it should not be a problem that it shows up using a rectangle. (Compare to#23914 where probably we do not want the Eiffel tower to show up in a legend...)

Regarding linestyle, PatchCollection (or maybe even collections in general) do return the dash pattern rather than the actual linestyle ('--'/'dashed'). This is sorted out in#23056 but that is not merged yet. I think the best way is to use something likenp.allclose([11.1, 4.8], ...) as there is some sort of floating-point error from the scaling.

timhoffm and FarukFS reacted with thumbs up emoji

@FarukFS
Copy link
Author

Oops. Will fix the test soon and make final changes to finish the PR.

@QuLogic
Copy link
Member

Just checking in on the status here? It seems almost complete.

@rcomer
Copy link
Member

@FarukFS are you still interested in working on this one?

@rcomer
Copy link
Member

I think it would be good to get this over the line, so I took the liberty of rebasing and fixing the test. The only difference is that the test now checks the handle linestyle against the collection linestyle instead of specific numbers (which will depend on the rcParams).

@rcomer
Copy link
Member

Removed some trailing whitespace and added the handler in the pyi file. Hopefully now CI is happy...

@rcomerrcomer marked this pull request as ready for reviewNovember 18, 2023 19:52
Comment on lines 435 to 438
lw = orig_handle.get_linewidths()[0]
dashes = orig_handle._us_linestyles[0]
facecolor = orig_handle.get_facecolor()[0]
edgecolor = orig_handle.get_edgecolor()[0]
Copy link
Member

Choose a reason for hiding this comment

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

Should probably do the same asHandlerPolyCollection and handle the empty case (i.e. not require the index 0 element to exist)

Copy link
Member

@QuLogicQuLogic left a comment

Choose a reason for hiding this comment

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

Any idea why none of the new code appears to have coverage, even though there's a new test?

Comment on lines 1 to 2
Legend handler for PatchCollection objects
---------------------------------------------------------------------
Copy link
Member

Choose a reason for hiding this comment

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

Should match underline to title length.

Comment on lines 14 to 17
p1, p2 = Polygon([[0,0],[100,100],[200,0]]), Polygon([[400,0],[500,100],[600,0]])
p3, p4 = Polygon([[700,0],[800,100],[900,0]]), Polygon([[1000,0],[1100,100],[1200,0]])
p = PatchCollection([p1,p2], label="a", facecolors='red', edgecolors='black')
p2 = PatchCollection([p3,p4], label="ab", color='green')
Copy link
Member

Choose a reason for hiding this comment

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

Missing spaces after commas.

story645
story645 previously requested changesNov 22, 2023
@rcomer
Copy link
Member

Hmmm. Seems this wasn’t as close to done as I’d assumed.

@rcomer
Copy link
Member

Any idea why none of the new code appears to have coverage, even though there's a new test?

At least some of that was becauselegend didn't get called in the test, socreate_artists didn't get called.

@rcomerrcomerforce-pushed themy-feature branch 3 times, most recently from7ed9875 tob5636e0CompareNovember 22, 2023 21:13
@rcomer
Copy link
Member

I think this one was ready to go but, since I pushed to it, I do not think I should approve it. If it's not ready I think it should be marked as orphaned.

@@ -427,6 +440,32 @@ def create_artists(self, legend, orig_handle,
return [legline]


class HandlerPatchCollection(HandlerPatch):
"""
Handler for `.PatchCollection` instances.
Copy link
Member

Choose a reason for hiding this comment

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

We should document that all properties are taken from the first patch.

Comment on lines +457 to +466
def create_artists(self, legend, orig_handle,
xdescent, ydescent, width, height, fontsize, trans):

p = self._create_patch(legend, orig_handle,
xdescent, ydescent, width, height, fontsize)

self.update_prop(p, orig_handle, legend)
p.set_transform(trans)

return [p]
Copy link
Member

Choose a reason for hiding this comment

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

This appears to be identical toHandlerPatch.create_artists, so can be left out.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@QuLogicQuLogicQuLogic left review comments

@ksundenksundenksunden left review comments

@timhoffmtimhoffmtimhoffm left review comments

@rcomerrcomerrcomer left review comments

@github-actionsgithub-actions[bot]github-actions[bot] left review comments

@story645story645story645 left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Projects
Status: Needs review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Labels for PatchCollection do not show
7 participants
@FarukFS@oscargus@timhoffm@rcomer@QuLogic@story645@ksunden

[8]ページ先頭

©2009-2025 Movatter.jp