Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

ENH: Add baseline feature to stackplot. #1517

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
5 participants
Contributor

Tillsten commented Nov 19, 2012

This PR enhances the stacked plot method with some three addition baseline calculations.

All follows from:
http://www.leebyron.com/else/streamgraph/

see the code (bsd) and the paper there.

Contributor

Tillsten commented Nov 19, 2012

To plot an nice example use:

    def layers(n, m):
        def bump(a):
            x = 1 / (.1 + r.random())
            y = 2 * r.random() - .5
            z = 10 / (.1+ r.random())
            for i in range(m):
                w = (i / float(m) - y) * z
                a[i] += x * np.exp(-w*w)
        a=np.zeros((m, n))
        for i in range(n):
            for j in range(5):            
                bump(a[:,i])
        return a 


    d=layers(3,100)
    clf()
    subplot(221)
    stackplot(gca(), range(100), d.T, baseline='zero')
    subplot(222)
    stackplot(gca(), range(100), d.T, baseline='sym')
    subplot(223)
    stackplot(gca(), range(100), d.T, baseline='wiggle')
    subplot(224)
    stackplot(gca(), range(100), d.T, baseline='weighted_wiggle')

@dmcdougall dmcdougall commented on an outdated diff Nov 19, 2012

lib/matplotlib/stackplot.py
# Color between array i-1 and array i
for i in xrange(len(y)-1):
r.append(axes.fill_between(x, y_stack[i,:], y_stack[i+1,:], facecolor=axes._get_lines.color_cycle.next(), **kwargs))
- return r
+ return r
@dmcdougall

dmcdougall Nov 19, 2012

Member

You removed (probably accidentally) the new line at the end of this file. Could you add it back in?

Member

dmcdougall commented Nov 19, 2012

Thanks @Tillsten. I haven't had a chance to play with this yet, I'll give more detailed feedback once I have. At the very least you should check this code adheres to the PEP8 standard, and you should add some image comparison tests for the new functionality.

Contributor

Tillsten commented Nov 20, 2012

@dmcdougall I don't see any PEP8 violations, except the newline issue, maybe you can give me a hint?

I have some questions about the image comparison:
Do i have to include the image itself or will it be generated if not found?

Member

dmcdougall commented Nov 20, 2012

@Tillsten I wasn't saying there was any, I was just asking you to check :)

When you write the test, run it. The test will fail because there is no baseline image to compare the output images to. The output images from that test run will be used as the baseline images for subsequent tests. I'm not sure that was entirely clear. Let me know if you need more help. Check out the coding guideline if you haven't already.

Contributor

Tillsten commented Nov 21, 2012

@dmcdougall After my last disastrous PR i had my eye on PEP8 :).

So if there no baseline image, a new one will be made? So i have just to make
a simple plot script with the right decorator?

btw, the link to http://matplotlib.org/devel/testing.html#testing does not work.

Contributor

Tillsten commented Nov 25, 2012

Hmm, it seems i can not run the tests without building matplotlib itself, which is hard on my windows machine. I get

  from matplotlib._path import (affine_transform, count_bboxes_overlapping_bbox,
  ImportError: No module named _path

. Can somebody else check and upload the baseline image?

Contributor

Tillsten commented Nov 25, 2012

@dmcdougall except the baseline image issue, i think this one is ready to merge

Contributor

Tillsten commented Dec 10, 2012

anything i can do?

@WeatherGod WeatherGod and 2 others commented on an outdated diff Dec 11, 2012

lib/matplotlib/stackplot.py
+ center[i] = center[i - 1]
+ for j in range(m):
+ if i == 0:
+ increase = y[j, i]
+ moveUp = 0.5
+ else:
+ belowSize = 0.5 * y[j, i]
+ for k in range(j + 1, m):
+ belowSize += y[k, i]
+ increase = y[j, i] - y[j, i - 1]
+ moveUp = belowSize / total[i]
+ center[i] += (moveUp - 0.5) * increase
+ first_line = center - 0.5 * total
+ y_stack += first_line
+ else:
+ raise ValueError('unknown baseline method')
@WeatherGod

WeatherGod Dec 11, 2012

Member

As part of this exception message, I would include what value was unknown.

@Tillsten

Tillsten Dec 12, 2012

Contributor

Is there any kind of standard message for this kind of error?

@dmcdougall

dmcdougall Dec 12, 2012

Member

No there is no 'standard' message. But a user could easily run into this error by, for example, making a typo in the baseline method name. Perhaps something more informative would be:

'Unknown baseline method %s' % baseline

Or:

'Baseline method %s not recognised. Expected 'zero', 'sym', 'wiggle' or 'weighted_wiggle'

Note, I am not necessarily suggesting the accepted baseline methods are hard-coded into the exception message. There's probably a better way to do it.

@Tillsten

Tillsten Dec 12, 2012

Contributor

@dmcdougall I think putting the acceptable methods into the message is fine in this case. They will be quite stable and if another method is added it is only a small change.

@WeatherGod WeatherGod and 1 other commented on an outdated diff Dec 11, 2012

lib/matplotlib/tests/test_axes.py
@@ -813,7 +813,8 @@ def test_log_scales():
ax.set_xscale('log', basex=9.0)
-@image_comparison(baseline_images=['stackplot_test_image'])
+@image_comparison(baseline_images=['stackplot_test_image',
+ 'stackplot_adv_test_image'])
@WeatherGod

WeatherGod Dec 11, 2012

Member

Where is this new image? I don't see it in this PR.

@Tillsten

Tillsten Dec 12, 2012

Contributor

I am under windows and i am not able to build mpl, so i can't run the test suite to generate the image. I could run the code with an old mpl, but i am afraid that the result is not right.

Contributor

Tillsten commented Jan 10, 2013

I removed the testimage (because i can't build master under my windows installtion) for now to make this PR ready
to merge.

Contributor

Tillsten commented Jan 10, 2013

I suspect the travis failure is not my fault?

Owner

mdboom commented Jan 10, 2013

Yes -- the Travis failure is a false negative -- that happens all too often unfortunately. I'd prefer to get the test in here before we merge -- I'll try to look into this later today. If we merge without it, let's at least create another issue to track it so we don't forget to put it in.

@NelleV NelleV commented on an outdated diff Jan 11, 2013

lib/matplotlib/stackplot.py
# Color between x = 0 and the first array.
- r.append(axes.fill_between(x, 0, y_stack[0, :],
- facecolor=axes._get_lines.color_cycle.next(), **kwargs))
+ r.append(axes.fill_between(x, first_line, y_stack[0,:],
@NelleV

NelleV Jan 11, 2013

Contributor

PEP8 compliance: there is a space missing between , and :

@NelleV NelleV commented on an outdated diff Jan 11, 2013

lib/matplotlib/stackplot.py
# Color between array i-1 and array i
- for i in xrange(len(y) - 1):
- r.append(axes.fill_between(x, y_stack[i, :], y_stack[i + 1, :],
- facecolor=axes._get_lines.color_cycle.next(), **kwargs))
+ for i in xrange(len(y)-1):
+ r.append(axes.fill_between(x, y_stack[i,: ], y_stack[i + 1, :],
@NelleV

NelleV Jan 11, 2013

Contributor

PEP8 compliance: there is a space missing between , and :, and there shouldn't be a space after :

@NelleV NelleV commented on an outdated diff Jan 11, 2013

lib/matplotlib/stackplot.py
# Color between array i-1 and array i
- for i in xrange(len(y) - 1):
- r.append(axes.fill_between(x, y_stack[i, :], y_stack[i + 1, :],
- facecolor=axes._get_lines.color_cycle.next(), **kwargs))
+ for i in xrange(len(y)-1):
@NelleV

NelleV Jan 11, 2013

Contributor

I'd add spaces around the operator here, even thought PEP8 now accepts this

@NelleV NelleV commented on an outdated diff Jan 11, 2013

lib/matplotlib/stackplot.py
# Assume data passed has not been 'stacked', so stack it here.
y_stack = np.cumsum(y, axis=0)
r = []
+ if baseline == 'zero':
+ first_line = 0.
+
+ elif baseline == 'sym':
+ first_line = -np.sum(y, 0) * 0.5
@NelleV

NelleV Jan 11, 2013

Contributor

PEP8: there is an extra space here

@NelleV NelleV and 1 other commented on an outdated diff Jan 11, 2013

lib/matplotlib/stackplot.py
+ first_line = (y * (m - 0.5 - np.arange(0, m)[:, None])).sum(0)
+ first_line /= -m
+ y_stack += first_line
+
+ elif baseline == 'weighted_wiggle':
+ #TODO: Vectorize this stuff.
+ m, n = y.shape
+ center = np.zeros(n)
+ total = np.sum(y, 0)
+ for i in range(n):
+ if i > 0:
+ center[i] = center[i - 1]
+ for j in range(m):
+ if i == 0:
+ increase = y[j, i]
+ moveUp = 0.5
@NelleV

NelleV Jan 11, 2013

Contributor

This name doesn't follow the pep8 convention: maybe replace it by move_up or moveup. Same with belowSize

@Tillsten

Tillsten Jan 11, 2013

Contributor

I used these names because the code i based it on (see first post) used them. So theres a point to for not changing the name but i dont really care.

@NelleV NelleV commented on an outdated diff Jan 11, 2013

lib/matplotlib/stackplot.py
# Color between array i-1 and array i
- for i in xrange(len(y) - 1):
- r.append(axes.fill_between(x, y_stack[i, :], y_stack[i + 1, :],
- facecolor=axes._get_lines.color_cycle.next(), **kwargs))
+ for i in xrange(len(y)-1):
+ r.append(axes.fill_between(x, y_stack[i,: ], y_stack[i + 1, :],
+ facecolor=axes._get_lines.color_cycle.next(),
@NelleV

NelleV Jan 11, 2013

Contributor

This line is slightly too long for pep8 (80 character instead of 79)

Contributor

NelleV commented Jan 11, 2013

@Tillsten I've written down the pep8 problems. In order to more easily see those, you can use the tool pep8, or the tool flake8 (I use flake8 because it also underlies pyflakes problems, which is very useful). If you use vim or emacs, you can easily have flake8 run automatically on your code.

Contributor

Tillsten commented Jan 11, 2013

@NelleV i used a pyflakes, but i think i lost my last (only pep8) commit on rebase.

@NelleV NelleV commented on the diff Jan 15, 2013

lib/matplotlib/stackplot.py
@@ -25,6 +25,16 @@ def stackplot(axes, x, *args, **kwargs):
Keyword arguments:
+ *baseline* : ['zero', 'sym', 'wiggle', 'weighted_wiggle']
+ Method use to calculate to baseline. 'zero' is just a
@NelleV

NelleV Jan 15, 2013

Contributor

Maybe I am misunderstanding, but shouldn't it be "Method used to calcule the baseline."?

@NelleV NelleV commented on the diff Jan 15, 2013

lib/matplotlib/stackplot.py
@@ -25,6 +25,16 @@ def stackplot(axes, x, *args, **kwargs):
Keyword arguments:
+ *baseline* : ['zero', 'sym', 'wiggle', 'weighted_wiggle']
+ Method use to calculate to baseline. 'zero' is just a
+ simple stacked plot. 'sym' is symmetric around zero and
+ is sometimes called `ThemeRiver`. 'wiggle' minimizes the
+ sum of the squared slopes. 'weighted_wiggle' does the
+ same but weights to account for size of each layer.
+ It is also called `Streamgraph`-layout. More details
+ can be found in http://www.leebyron.com/else/streamgraph/.
@NelleV

NelleV Jan 15, 2013

Contributor

"can be found at http"

Owner

mdboom commented Jan 16, 2013

This needs a test and a CHANGELOG entry.

Contributor

Tillsten commented Jan 16, 2013

@mdboom This had a test, my problem is that i am under windows and i am not able to run master. So uploading a
comparsion image with this PR dont make much sense. Because nobody was able to help me, i removed the test from this PR to make it ready to merge.

Member

dmcdougall commented Jan 16, 2013

@Tillsten I can make a PR against your branch to add a test for you. Or I can fork your branch directly and create a new PR. Any preference?

Contributor

Tillsten commented Jan 16, 2013

@dmcdougall Just fork it (and possible take care of the spelling errors). i am not at the right pc at the moment.
You can use the code in the first post to generate a passable image (of course, set the seed).

@dmcdougall dmcdougall referenced this pull request Jan 16, 2013

Merged

Feature stack base #1671

Member

dmcdougall commented Jan 16, 2013

See #1671.

Contributor

Tillsten commented Jan 16, 2013

@dmcdougall Thanks! closing this one.

@Tillsten Tillsten closed this Jan 16, 2013

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