-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Add capstyle and joinstyle attributes to Collection class (Issue #8277) #9523
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Unless I am mistaken, this does not actually cause the capstyle and joinstyle to be used while drawing. You probably need to update Collection.draw accordingly. Note that unless you mess with Renderer.draw_path_collection, the capstyle and joinstyle will be global ones (not settable per-item). On the one hand, who wants to set these per item? On the other hand, even antialiasing is settable per-item. |
Hi @anntzer, I did update change Yes, I was also wondering if it should be possible to set cap- and joinstyle for each element in the collection, but as I understand it, this is not the solution proposed in the issue. |
Sorry, I was not careful when proofreading. Indeed they are set correctly. Allowing per-item setting is a much more complicated piece of work, so I think I'm OK with not having it for now. I think this warrants an image test (to show that the styles are actually propagated to the draw), and a whatsnew entry. |
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 a lot for this patch! I have a few minor comments. I'm happy to create a PR on your branch or to push the changes directly to your branch if you prefer not fixing those.
@@ -104,6 +104,8 @@ def __init__(self, | |||
facecolors=None, | |||
linewidths=None, | |||
linestyles='solid', | |||
capstyle=None, |
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.
Those two new arguments need to be documented in the docstring for this class.
lib/matplotlib/collections.py
Outdated
""" | ||
Set the capstyle for the collection. | ||
|
||
ACCEPTS: ['butt' | 'round' | 'projecting'] |
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.
We're using numpydoc style docstring. Can you delete this line, but add this to the parameters list? (See comment bellow).
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.
Until we actually get rid of the kwdoc mechanism (whether we do so is a separate issue), I would prefer leaving the ACCEPTS strings there, as it avoids having an ugly property -- unknown
entry in the rendered table. (But that does not preclude documenting them in the parameters table below too -- I agree with @NelleV on that part.)
Same comment applies throughout.
lib/matplotlib/collections.py
Outdated
|
||
Parameters | ||
---------- | ||
cs : The capstyle |
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.
Considering we are using numpydoc, that should be:
cs : ['butt' | 'round' | 'projecting']
the capstyle
lib/matplotlib/collections.py
Outdated
""" | ||
Set the joinstyle for the collection. | ||
|
||
ACCEPTS: ['miter' | 'round' | 'bevel'] |
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.
Same here :)
Keyword arguments and default values: | ||
|
||
* *edgecolors*: None | ||
* *facecolors*: None | ||
* *linewidths*: None | ||
* *capstyle*: None | ||
* *joinstyle*: None |
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.
👍
Sorry for the noise, I had some problems with my test setup and missed the PEP8 warnings. |
So is there anything left to do on this? |
doc/users/whats_new.rst
Outdated
@@ -246,6 +246,13 @@ volumetric model. | |||
Improvements | |||
++++++++++++ | |||
|
|||
Add `capstyle` and `joinstyle` attributes to `Collection` |
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.
capstyle and joinstyle should use two backquotes on both sides (this is somewhat poorly documented at http://matplotlib.org/devdocs/devel/documenting_mpl.html#formatting but the idea is that single backquotes mean "a python object that has its own entry in the docs", which is the case for Collection but not for capstyle and joinstyle, which are just parameters).
Same below.
|
||
@image_comparison(baseline_images=['capstyle'], | ||
extensions=['png']) | ||
def test_capstyle_image(): |
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.
Can you join the two image comparisons into a single test and thus a single reference image? Reference test images are a large part of why the repo is so heavy, so we're trying to be careful with adding more of them.
After doing so, you can squash the old images out of the history with rebase; if you're not comfortable with it I can take care of it when merging too, just let me know.
Just some minor stuff. The rest looks good. |
8b22d5e
to
36b9c46
Compare
OK, fixed now. |
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.
LGTM :-)
Thanks for the contribution @simonpf ! |
@@ -246,6 +246,13 @@ volumetric model. | |||
Improvements | |||
++++++++++++ | |||
|
|||
Add ``capstyle`` and ``joinstyle`` attributes to `Collection` |
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.
This got put into the 2.1 whats_new 😞
PR Summary
Adds
_capstyle
and_joinstyle
attributes as well as corresponding setters toCollection
class. Changesdraw
class method to set capstyle and joinstyle of renderer accordingly. This solves the problem of not being able to set capstyle and joinstyle ofLineCollection
(Issue #8277).PR Checklist
Example