Skip to content
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

Add EventCollection and eventplot #1657

Merged
merged 0 commits into from Jan 16, 2013
Merged

Add EventCollection and eventplot #1657

merged 0 commits into from Jan 16, 2013

Conversation

toddrjen
Copy link
Contributor

As discussed in the mailing list, here is the event plot. It's purpose it to plot one or more series of 1D data points along horizontal or vertical lines. It is dived into two parts.

The first part is the EventCollection collections class, which is a subclass of LineCollection that makes it easy to create and manipulate individual rows of vertical lines or columns of horizontal lines. Besides being used in the final plot type, this is also useful on its own, for example for marking the locations of data points along an axis (see eventcollection_demo.py). Besides the convenience functions, since it is a subclass of LineCollection it also exposes all internal LineCollection and Collection methods, allowing more fine-grained modifications for artists.

The second part is an axes method (also in pyplot through boilerplate.py) which is given one or more rows of data points and creates corresponding rows or columns of lines. All rows or columns can be given the same line properties or individual line properties (see eventplot_demo.py). It returns a list of EventCollection objects.

Tests for both EventCollection and eventplot are present, and the files have been tested for pep8 compliance and run through pyflakes and pylint and fixed wherever appropriate.

linestyles = [linestyles] * len(positions)

if len(lineoffsets) != len(positions):
raise ValueError('lineoffsets and positions are unequal sized \
Copy link
Member

Choose a reason for hiding this comment

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

Minor nitpick: this way of setting strings will indent the output in a weird way. There are several ways to avoid this, my favourite being:

raise ValueError("lineoffsets and positions are unequal sized "
                 "sequences.")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I will fix that, but I will hold off committing it for a few
days to see if there are more substantial changes that should be done at
the same time.

@WeatherGod
Copy link
Member

You don't need to commit the png versions of the svg and pdf files. Please remove the files that end in "_pdf.png" and "_svg.png".

@toddrjen
Copy link
Contributor Author

Okay, I will do so.

@NelleV
Copy link
Member

NelleV commented Jan 14, 2013

Very nice patch ! Both the code and documentation are very clear and very clean.

@toddrjen
Copy link
Contributor Author

Thank you very much! I was concerned the documentation was not clear
enough.

@toddrjen
Copy link
Contributor Author

I have made the recommended changes

@toddrjen
Copy link
Contributor Author

One remaining issue is all the whitespace in the generated pyplot code. Should that be left in, removed manually, or should I modify boilerplate.py to remove it automatically?

I already have a modified version of boilerplate.py that strips all trailing spaces automatically, I could commit it if that is the best approach.

@mdboom
Copy link
Member

mdboom commented Jan 15, 2013

I'd be pro modifying boilerplate.py to remove extra whitespace automatically, but let's not do it as part of this PR. Best to just include the generated pyplot.py code as it is for this PR, then we can fix boilerplate.py and commit a cleaner pyplot.py along with it.

@dmcdougall
Copy link
Member

I agree with @mdboom, with the view that modifying boilerplate.py to produce PEP8 compliant code would be the end goal.

@toddrjen
Copy link
Contributor Author

Okay, then all suggestions so far have been implemented as far as I can
see. Please let me know what else you thinks should be modified.

@dmcdougall
Copy link
Member

@toddrjen Unfortunately, you'll need to rebase this against current master so that it will merge cleanly.

@toddrjen toddrjen merged commit 39384a5 into matplotlib:master Jan 16, 2013
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.

None yet

5 participants