Feature stack base #1671

dmcdougall · 2013-01-16T20:25:16Z

New version of #1517 with a test, typos fixed, and a CHANGELOG.

dmcdougall · 2013-01-16T20:40:58Z

@Tillsten Are you happy with the CHANGELOG and the whats_new?

dmcdougall · 2013-01-16T21:51:40Z

@Tillsten Instead of embedding an image to the whats_new file, I'll write an example and just refer to that. I think that's cleaner.

dmcdougall · 2013-01-16T23:31:40Z

Ok, I think this is good to go now. Tests pass locally except for tight_bbox (which has nothing to do with the stackplot baseline test, it's a weird font thing I occasionally get):

Expected:

Computed:

Difference:

tacaswell · 2013-01-17T00:27:08Z

I have also been intermittently getting this failure. (python2.7)

dmcdougall · 2013-01-17T00:34:34Z

@tacaswell Specifically on this branch, or in general?

tacaswell · 2013-01-17T00:38:01Z

@dmcdougall in general (typically with in a few commits of master).

Sorry I wasn't clear.

Tillsten · 2013-01-17T00:57:53Z

@dmcdougall Just thanks for doing the final touches!

dmcdougall · 2013-01-17T01:04:15Z

@Tillsten No problem.

Feedback permitting, someone else should push the big green button!

pelson · 2013-01-17T09:17:06Z

lib/matplotlib/tests/test_axes.py

@@ -863,6 +863,41 @@ def test_stackplot():
    ax.set_xlim((0, 10))
    ax.set_ylim((0, 70))

+
+@image_comparison(baseline_images=['stackplot_test_baseline'])


Think we need to "remove_text" from the test results (to avoid freetype version differences across architectures)

Yes! I think I meant to do this and forgot. Thanks.

pelson · 2013-01-17T09:21:01Z

Excellent feature. Only a few minor comments, but I think it looks great!

dmcdougall · 2013-01-17T19:18:40Z

lib/matplotlib/stackplot.py

+
+        for i in range(1, n):
+            center[i] = center[i - 1]
+            center[i] += np.dot(move_up[:, i], increase[:, i])


@Tillsten I used your idea to construct below_size and it works nicely! But now I'm stuck here. Any ideas on how to move out the center with that inter-dependence inside the loop? I tried modifying your approach but it didn't work.

Are you sure my code dient work? I also fester the code and the max difff was 1e-17 maybe enough to let it fail. I am at my phone at the moment but I think using multiply, sum and than cumsum should do the trick.

I copied and pasted verbatim into the else block, replacing all I had there already. Maybe I fluffed it up. You could try it independently and run the test for feedback. That would actually be helpful.

dmcdougall · 2013-01-17T22:11:27Z

Wooooo vectorised!

Thanks for you help and advice @Tillsten. Do you like the proposed solution? Is there anything you'd like to add/change before it is rebased and merged?

Tillsten · 2013-01-18T14:59:46Z

@dmcdougall I checked both versions, both are getting excat the same result on my machine. I prefer mine, because
your diag generates an n-n-matrix, which could be quite big.

edit: I am wrong, it just extract the diagonal (the dot product is (m, m)). But still, some unnecessary terms are calculated. Some simple timing shows a 2.5 increase for my method.

Tillsten · 2013-01-18T15:11:23Z

Also i was able to simplify below_size.

stack = np.cumsum(y, axis=0)
m, n = y.shape
center = np.zeros(n)
total = np.sum(y, 0)
increase = np.hstack((y[:, 0:1], np.diff(y)))
below_size = total - nstack
below_size += 0.5*y
move_up = below_size / total
move_up[:, 0] = 0.5
center = (move_up - 0.5) * increase
center = np.cumsum(center.sum(0))
first_line = center - 0..5 * total
stack += first_line

Tillsten · 2013-01-18T15:16:28Z

Test is here:

https://www.wakari.io/usermgmt/nb/tillsten/Vectorizing_the_loop

Also note that this just probably premature optimizing, but vetorizing stuff is surprisingly fun :)

dmcdougall · 2013-01-18T15:24:35Z

@Tillsten Ok. Your method is faster, so let's use that! I must've fluffed something up in the copy-paste. Thanks for collaborating with me. I'll update the PR with your approach.

Also note that this just probably premature optimizing, but vetorizing stuff is surprisingly fun :)

Yes it is! What's more fun is to work with somebody on it. Thanks for all your hard work and guidance, I really appreciate it.

dmcdougall · 2013-01-18T15:28:46Z

@Tillsten Out of interest, would you be able to add a timing test comparing against the origin double for loop too? Not for any reason other than I am curious.

dmcdougall · 2013-01-18T15:38:50Z

@Tillsten I get a local test failure with your patch (implemented in c356f44). Could you perhaps take a look? Here's the diff from the test output.

Expected:

Computed:

Diff:

Tillsten · 2013-01-18T15:44:04Z

@dmcdougall After including the orginal loop, i am very happy that we verctorize it. There is a factor of 10 000 of differnce.

dmcdougall · 2013-01-18T15:45:09Z

There is a factor of 10 000 of differnce.

Down comes the house! That's what I like to see. Good work. Tests are now passing locally, too.

dmcdougall · 2013-01-18T15:47:22Z

I'm going to rebase this pull request so it will merge cleanly. During that process, I will squash down the vectorisation commits into a single vectorisation commit. I'll then check for PEP8 compliancy. After that I'll run the tests locally and report back.

After that, this should be good to go.

Tillsten · 2013-01-18T15:48:49Z

I have not too experience with the workflow, but why not squash everything together?

dmcdougall · 2013-01-18T15:54:51Z

@Tillsten I can do that, too. The reason I did it this way is because then each commit adds something logical and complete in bite-size chunks. This makes tools like git bisect easier to use should something go wrong.

Perhaps this is better thrown out for a consensus. @pelson and @mdboom What do you guys think?

WeatherGod · 2013-01-18T16:11:20Z

My personal feelings about squashing commits is to be conservative with
it. Squashing everything together makes for very big diffs, but not
squashing can lead to many confusing commit messages and clutter
histories. Squashing the vectorization commits makes sense here.

dmcdougall · 2013-01-20T18:35:24Z

This seems reasonably stable now, so I'll merge it if there's no more feedback by, say, tomorrow. It'd be good to get this into master so people can play with it.

pelson · 2013-01-21T11:04:22Z

examples/pylab_examples/stackplot_demo2.py

+import matplotlib.pyplot as plt
+
+np.random.seed(0)
+def layers(n, m):


I know its only an example, but a docstring would be nice here - something which focuses on the purpose of the function, rather than its args/kwargs.

Actually, this is not uncommon to do in the examples. IIRC, the sphinx
doc-builder will take the docstring portion of the example, and render it
as ReST text above the source code portion. If one feels like having a
docstring there, feel free. The more the better.

Sure thing :)

Explanation of the code:
This is just a direct copy of the original code. All it does is calculating random Gaussians.

Done in bd204e1.

pelson · 2013-01-21T11:09:40Z

This is an excellent PR, and from my point of view, is on the cusp of being merged. I've added two questions/actions - one of which I would appreciate others either agreeing, or throwing it out of the park.

Cheers,

Tillsten · 2013-01-25T14:17:33Z

merge it?

dmcdougall · 2013-01-26T03:15:17Z

@Tillsten I would rather wait until @pelson confirms his concerns have been addressed before merging.

In general I like to wait for at least one of the other developers to give it a once-over, but I realise that's not always possible with people being busy.

dmcdougall · 2013-01-26T03:17:25Z

Also, the tests that were added here are now failing. Curious.

dmcdougall · 2013-01-26T03:20:13Z

Locally I don't get the stack_base errors with python 2.7. The error bar and tight_bbox failures have nothing to do with these changes.

This boasts an O(10^4) speed up

Also made a minor PEP8 change

dmcdougall · 2013-01-26T03:24:21Z

Rebasing addresses the error_bar failures locally. Now all tests pass locally with py2.7. Travis will probably have a different story.

Feature stack base

dmcdougall mentioned this pull request Jan 16, 2013

ENH: Add baseline feature to stackplot. #1517

Closed

pelson reviewed Jan 17, 2013
View reviewed changes

dmcdougall reviewed Jan 17, 2013
View reviewed changes

tacaswell mentioned this pull request Jan 19, 2013

Remove figure from Gcf when it is closed #1683

Closed

pelson reviewed Jan 21, 2013
View reviewed changes

Tillsten and others added 9 commits January 25, 2013 21:20

[ENH] added baseline feature to stacked graph.

24f537f

pep8 compliance

b21b348

Fix typos

6f7a769

Add stackplot baseline demo

6975bcb

Update changelog and whats_new

ff8cce4

Add stackplot baseline test

6dc5887

Remove text from regression test output

f318d39

Vectorise the loops

339ca4e

This boasts an O(10^4) speed up

Add docstring to example funtion

6b4678b

Also made a minor PEP8 change

dmcdougall added a commit that referenced this pull request Jan 27, 2013

Merge pull request #1671 from dmcdougall/feature_stack_base

daf551d

Feature stack base

dmcdougall merged commit daf551d into matplotlib:master Jan 27, 2013

dmcdougall deleted the feature_stack_base branch January 27, 2013 19:49

Feature stack base #1671

Feature stack base #1671

Conversation

dmcdougall commented Jan 16, 2013

dmcdougall commented Jan 16, 2013

dmcdougall commented Jan 16, 2013

dmcdougall commented Jan 16, 2013

tacaswell commented Jan 17, 2013

dmcdougall commented Jan 17, 2013

tacaswell commented Jan 17, 2013

Tillsten commented Jan 17, 2013

dmcdougall commented Jan 17, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pelson commented Jan 17, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmcdougall commented Jan 17, 2013

Tillsten commented Jan 18, 2013

Tillsten commented Jan 18, 2013

Tillsten commented Jan 18, 2013

dmcdougall commented Jan 18, 2013

dmcdougall commented Jan 18, 2013

dmcdougall commented Jan 18, 2013

Tillsten commented Jan 18, 2013

dmcdougall commented Jan 18, 2013

dmcdougall commented Jan 18, 2013

Tillsten commented Jan 18, 2013

dmcdougall commented Jan 18, 2013

WeatherGod commented Jan 18, 2013

dmcdougall commented Jan 20, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pelson commented Jan 21, 2013

Tillsten commented Jan 25, 2013

dmcdougall commented Jan 26, 2013

dmcdougall commented Jan 26, 2013

dmcdougall commented Jan 26, 2013

dmcdougall commented Jan 26, 2013