Add EventCollection and eventplot #1669

Merged
merged 13 commits into from Feb 21, 2013

Conversation

Projects
None yet
8 participants
Contributor

toddrjen commented Jan 16, 2013

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.

Contributor

toddrjen commented Jan 16, 2013

I did something wrong and closed the last pull request.

This version has been rebased so it should apply cleanly.

Contributor

toddrjen commented Feb 1, 2013

Is there anything else that needs to be done here? I just tested this and it seems to merge cleanly into master. If everything is okay can someone please merge this when they have a chance? Thanks.

There are some other changes I would like to work on but I want to clean up my fork completely before I do so.

@mdboom mdboom and 3 others commented on an outdated diff Feb 1, 2013

lib/matplotlib/tests/test_collections.py
+ 'extra_positions': extra_positions,
+ 'orientation': orientation,
+ 'lineoffset': lineoffset,
+ 'linelength': linelength,
+ 'linewidth': linewidth,
+ 'color': color,
+ 'linestyle': linestyle,
+ 'antialiased': antialiased
+ }
+ splt.set_xlim(-1, 22)
+ splt.set_ylim(0, 2)
+ return splt, event_col, props
+
+
+@image_comparison(baseline_images=['EventCollection_plot__default'])
+def test__EventCollection__get_segments():
@mdboom

mdboom Feb 1, 2013

Owner

The double underscores here don't follow PEP8

@toddrjen

toddrjen Feb 1, 2013

Contributor

Are you certain? I just ran it through the pep8 checker and it didn't complain. I also don't see anything in pep8 regarding double underscores except at the beginning and end of names.

@mdboom

mdboom Feb 1, 2013

Owner

Ok. Maybe you're right -- I just always think of double underscores as indicating something "special", which these are not. @NelleV, as our PEP8 expert, what do you think?

@NelleV

NelleV Feb 1, 2013

Contributor

Strictly speaking the capital letters are not pep8 compliant, but I don't think double underscore matter in this case, because they don't surround the name (init etc). I do agree with you that it is a bit confusing and I would rather remove them, but this is really nitpicking :)

@NelleV

NelleV Feb 1, 2013

Contributor

I also don't think this should be a roadblock for this PR being merged.

@toddrjen

toddrjen Feb 1, 2013

Contributor

The reason I include them is because it makes it easier to separate out the
method names. So in this example:

test__EventCollection__get_segments

I think it makes it clearer you are testing the get_samples method of
the EventCollection class. At least to me this was less clear when it
was formatted like this:.

test_EventCollection_get_segments

Or

test_eventcollection_get_segments

That is why I went with this approach. I figured there wasn't a
problem with it since the pep8 checker didn't find any problems at all
with the file.

@pelson

pelson Feb 4, 2013

Member

To separate the namespace a bit, you could always create a test class with test methods (akin to unittest.TestCase but without the straitjacket)...

@toddrjen

toddrjen Feb 7, 2013

Contributor

I tried using a separate test class. However, the Matplotlib test
decorators do not work inside methods so I had to use the current
approach.

This is not a problem with my code, I could not find any examples of these
decorators being used in classes, Matplotlib tests only use methods of they
are not using the decorators.

@mdboom mdboom commented on an outdated diff Feb 1, 2013

examples/pylab_examples/eventcollection_demo.py
+ orientation='vertical')
+
+# add the events to the axis
+ax.add_collection(xevents1)
+ax.add_collection(xevents2)
+ax.add_collection(yevents1)
+ax.add_collection(yevents2)
+
+# set the limits
+ax.set_xlim([0, 1])
+ax.set_ylim([0, 1])
+
+ax.set_title('line plot with data points')
+
+# display the plot
+fig.show()
@mdboom

mdboom Feb 1, 2013

Owner

This should be plt.show to work with all backends.

@mdboom mdboom and 1 other commented on an outdated diff Feb 1, 2013

examples/pylab_examples/eventplot_demo.py
+fig = plt.figure()
+
+# create a horizontal plot
+ax1 = fig.add_subplot(211)
+ax1.eventplot(data, colors=colors, lineoffsets=lineoffsets,
+ linelengths=linelengths)
+ax1.set_title('horizontal eventplot')
+
+
+# create a vertical plot
+ax2 = fig.add_subplot(212)
+ax2.eventplot(data, colors=colors, lineoffsets=lineoffsets,
+ linelengths=linelengths, orientation='vertical')
+ax2.set_title('vertical eventplot')
+
+fig.show()
@mdboom

mdboom Feb 1, 2013

Owner

Again, plt.show

@toddrjen

toddrjen Feb 1, 2013

Contributor

Fixed

Owner

mdboom commented Feb 1, 2013

Thanks for pinging us about this. matplotlib

This looks pretty good, other than my minor comment.

This does reveal an unrelated bug in the Agg backend that snapping is not turned on for collections. The lines in these event plots are not getting pixel-snapped, and thus look rather "fuzzy" (at least to my eye). Would you mind also applying the following:

diff --git a/src/_backend_agg.cpp b/src/_backend_agg.cpp
index fb4a61f..7a5ea1e 100644
--- a/src/_backend_agg.cpp
+++ b/src/_backend_agg.cpp
@@ -1660,7 +1660,7 @@ RendererAgg::draw_path_collection(const Py::Tuple& args)

     try
     {
-        _draw_path_collection_generic<PathListGenerator, 0, 1>
+        _draw_path_collection_generic<PathListGenerator, 1, 1>
         (gc,
          master_transform,
          gc.cliprect,

and then regenerating the test baseline images.

This will bring the snapping behavior of this inline with the rest of matplotlib. If the user prefers to not have snapping, they can always pass snap=False to the eventplot method (just tested that, and indeed it works).

Contributor

toddrjen commented Feb 1, 2013

I have fixed the bug and submitted updated images for everything that was
changed

Owner

mdboom commented Feb 1, 2013

Ok. I think this is good to merge then. I'm going to leave it up for another day for others to comment. I think the Travis failures on this are the same as on master (which I'm still trying to get to the bottom of), so that shouldn't impact merging this.

Owner

mdboom commented Feb 1, 2013

Actually -- I take that back. There are a lot more failures here than on master right now. Perhaps rebasing on the current master may help to pick up any recent bugfixes there. I'll see what sort of test failures I'm getting locally with this as well.

Owner

mdboom commented Feb 1, 2013

I've found the issue: somehow pyplot.py was edited without updating boilerplate.py, and your rerunning of boilerplate.py here overwrote those changes. If you rebase on (or simply cherry-pick) 2188187, and then re-run boilerplate.py, I think we should have something here that is passing all tests (with the exception of the ones already failing on master).

@ghost

ghost commented Feb 1, 2013

I did as you suggested. I then compared the test results on master to this branch, and the failures are the same. I also re-tested the merge and it still worked.

It doesn't look like travis is going to update the test since it doesn't seem to recognize the squashing I did. But a local test had the same failures in master (confirmed up-to-date) and this branch. So everything seems to be fine.

Owner

mdboom commented Feb 1, 2013

Yeah -- it's not always clear to me when Travis chooses to pick up PRs and test them either. I'll leave this up for another 24 hours in case there are further comments, but otherwise, I think this is good for merging.

@ghost

ghost commented Feb 1, 2013

Great! Thanks a lot.

Member

pelson commented Feb 4, 2013

I'm not confident why this should have ~420 files changed, many of them test result images - is there a reason for doing this as the overall effect will be a massive increase in the repo's size (not necessarily the distribution size though)? If we could avoid doing such a thing, I think it would be worth doing.

Cheers,

Owner

mdboom commented Feb 4, 2013

I suspect this was an unintentional mistake (perhaps when doing the rebase). There's no reason I can see that all of the baseline images would require updating. @ghost: I can't figure out which of these commits is updating all of those files. Can you see if you can update this branch such that git diff upstream/master only shows the changes you actually made?

Contributor

toddrjen commented Feb 7, 2013

As you suggested I fixed the big regarding snapping.

However, this led to changes in many of the test images. So I then checked
which images had failing tests and replaced them with new versions that
include proper snapping. This is the reason for all the changes.

These should all be in the commit containing the bug fix, the last commit
in this pull request I believe.

Owner

mdboom commented Feb 7, 2013

Thanks for the explanation. When I make my suggested change (to _backend_agg.cpp) directly on master, I don't get any test failures, so I'm surprised it's failing for you. Let me have a closer look at the images to see what's up.

Owner

mdboom commented Feb 7, 2013

Sorry -- my bad. It was just an environmental problem on my end. I indeed see that with my change there are a number of changes to the test images (all improvements, IMHO), so it does make sense to update those images. We'll have to carefully coordinate with #1291, which also has some baseline image updates.

Owner

mdboom commented Feb 7, 2013

Travis seems to be failing only on the stackplot test -- which is a different failure than master is currently getting. This is, of course, very close, and thanks for all your hard work. I'll try to reproduce locally and see if anything obvious pops out.

Member

dmcdougall commented Feb 7, 2013

I indeed see that with my change there are a number of changes to the test images (all improvements, IMHO)

I disagree. See lib/matplotlib/tests/baseline_images/test_text/font_styles.png. This is the file that will replace the current file. This change introduces a regression.

Contributor

toddrjen commented Feb 18, 2013

This issue is not really directly related to these plot types. It looks
like there are multiple issues with the unit tests. It is probably not a
good idea to replace the reference figures until we know they are all
correct. Otherwise we might end up with unit tests that pass when they
should fail. But the bugfix requires replacing the reference figures,
otherwise a number of tests will fail. So I do not think this bugfix can go
in yet.

Would it be okay if I remove bugfix from this PR? That way we can get the
new plot types in, then we can have a more focused effort on fixing all the
problems with the unit tests. That will also make the merge history
cleaner.

Owner

efiring commented Feb 18, 2013

Your proposal makes sense to me. Snapping problems may be revealed by your PR, but they are separate, and should be dealt with separately. Maybe the ideal would be fix the snap problem, and update images as necessary, and then rebase and merge your PR, with sample images that are correct from the start. But this would be advisable only if the snap fix is imminent.

Contributor

toddrjen commented Feb 19, 2013

The problem is that there are a bunch of failing tests. I think these
should be fixed before merging the snapping fix.

Using the font example mentioned earlier, what if that problem is due to
another unrelated change? The new version of the figure is added, and we
end up with a test that should fail now passes. So we can't put in the new
version of the figure until the figure is correct.

We don't know how many such incorrect figures there may be, and now telling
how long it will be before they are all found fixed. And until they are all
fixed, I think it would be a very bad idea to merge the snapping fix.

That is why I would like to get these new figures merged now. There is no
telling how long it will be before all the unit tests are passing again.

Contributor

toddrjen commented Feb 21, 2013

I have removed the the snapping fix for the reasons mentioned previously.
I have also rebased, reran boilerplate.py, improved the tests, added some
more documentation, added another pair of examples, ran all the unit tests,
and tested the merge. Everything seems to be working. Unit tests failures
are the same as with master.

So would it be possible to go ahead and get this merged?

mdboom merged commit 93cbcb3 into matplotlib:master Feb 21, 2013

1 check failed

default The Travis build could not complete due to an error
Details

Just saw this in the 1.3.0 announcement, very nice! How about a feature, where the values on the y-axis are named items, like {'Event1': 10, 'Event2': 20} ?

Owner

tacaswell commented Aug 1, 2013

@michaelaye Can you make a new issue with that request?

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