Skip to content

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

Merged
merged 4 commits into from
Oct 26, 2017

Conversation

simonpf
Copy link

@simonpf simonpf commented Oct 22, 2017

PR Summary

Adds _capstyle and _joinstyle attributes as well as corresponding setters to Collection class. Changes draw class method to set capstyle and joinstyle of renderer accordingly. This solves the problem of not being able to set capstyle and joinstyle of LineCollection (Issue #8277).

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

Example

import matplotlib.pyplot as plt
from matplotlib.collections import LineCollection
from matplotlib import colors as mcolors

import numpy as np

x = np.arange(100)
ys = x[::10, np.newaxis] + (x + 10.0 * np.random.rand(*x.shape))[np.newaxis, :]

segs = np.zeros((10, 100, 2), float)
segs[:, :, 1] = ys
segs[:, :, 0] = x

segs = np.ma.masked_where((segs > 50) & (segs < 60), segs)
ax = plt.axes()
ax.set_xlim(x.min(), x.max())
ax.set_ylim(ys.min(), ys.max())

colors = [mcolors.to_rgba(c)
          for c in plt.rcParams['axes.prop_cycle'].by_key()['color']]
line_segments = LineCollection(segs, linewidths=(2, 3, 4, 5),
                               colors=colors, linestyle='solid')
line_segments.set_joinstyle("miter")
line_segments.set_capstyle("round")
ax.add_collection(line_segments)
ax.set_title('Line collection with masked arrays')
plt.show()

@simonpf simonpf changed the title Added capstyle and joinstyle attributes to Collection class (Issue #8277) Add capstyle and joinstyle attributes to Collection class (Issue #8277) Oct 22, 2017
@anntzer
Copy link
Contributor

anntzer commented Oct 22, 2017

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.

@simonpf
Copy link
Author

simonpf commented Oct 22, 2017

Hi @anntzer,

I did update change Collection.draw and tested this with the example given in the issue and the one above.

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.

@anntzer
Copy link
Contributor

anntzer commented Oct 22, 2017

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.

Copy link
Member

@NelleV NelleV left a 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,
Copy link
Member

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.

"""
Set the capstyle for the collection.

ACCEPTS: ['butt' | 'round' | 'projecting']
Copy link
Member

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).

Copy link
Contributor

@anntzer anntzer Oct 22, 2017

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.


Parameters
----------
cs : The capstyle
Copy link
Member

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

"""
Set the joinstyle for the collection.

ACCEPTS: ['miter' | 'round' | 'bevel']
Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

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

👍

@dstansby dstansby added this to the v2.2 milestone Oct 23, 2017
@simonpf
Copy link
Author

simonpf commented Oct 24, 2017

Sorry for the noise, I had some problems with my test setup and missed the PEP8 warnings.

@simonpf
Copy link
Author

simonpf commented Oct 26, 2017

So is there anything left to do on this?

@@ -246,6 +246,13 @@ volumetric model.
Improvements
++++++++++++

Add `capstyle` and `joinstyle` attributes to `Collection`
Copy link
Contributor

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():
Copy link
Contributor

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.

@anntzer
Copy link
Contributor

anntzer commented Oct 26, 2017

Just some minor stuff. The rest looks good.

@simonpf
Copy link
Author

simonpf commented Oct 26, 2017

OK, fixed now.

Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

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

LGTM :-)

@NelleV NelleV merged commit 3c3e8ba into matplotlib:master Oct 26, 2017
@NelleV
Copy link
Member

NelleV commented Oct 26, 2017

Thanks for the contribution @simonpf !

@@ -246,6 +246,13 @@ volumetric model.
Improvements
++++++++++++

Add ``capstyle`` and ``joinstyle`` attributes to `Collection`
Copy link
Member

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 😞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants