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 a cumsum transform to cumulatively sum a single column #7961

Merged
merged 6 commits into from Jun 5, 2018

Conversation

Projects
None yet
3 participants
@bryevdv
Copy link
Member

bryevdv commented Jun 5, 2018

I ran across the SO question Create pie-chart using bokeh with dictionary
and thought I woudl see if it could be done with Stack but soon realized that it could not, since Stack stacks multiple entire columns element-wise. It occured to me that a cummulative sum was a very useful but missing piece that would also be trivial to implement, so I threw together this PR.

I will add docs and tests in subsequent commits.

@bryevdv bryevdv added the status: WIP label Jun 5, 2018

@bryevdv

This comment has been minimized.

Copy link
Member Author

bryevdv commented Jun 5, 2018

With this PR the SO question can be achieved much more cleanly with:


from collections import Counter
from math import pi

import pandas as pd

from bokeh.palettes import Category20c
from bokeh.plotting import figure, show
from bokeh.transform import cumsum

x = Counter({
    'United States': 157,
    'United Kingdom': 93,
    'Japan': 89,
    'China': 63,
    'Germany': 44,
    'India': 42,
    'Italy': 40,
    'Australia': 35,
    'Brazil': 32,
    'France': 31,
    'Taiwan': 31,
    'Spain': 29
})

data = pd.DataFrame.from_dict(dict(x), orient='index').reset_index().rename(index=str, columns={0:'value', 'index':'country'})
data['angle'] = data['value']/sum(x.values()) * 2*pi
data['color'] = Category20c[len(x)]

p = figure(plot_height=350, title="Pie Chart", toolbar_location=None,
           tools="hover", tooltips=[("Country", "@country"),("Value", "@value")])

p.wedge(x=0, y=1, radius=0.4, 
        start_angle=cumsum('angle', include_zero=True), end_angle=cumsum('angle'),
        line_color="white", fill_color='color', legend='country', source=data)

p.axis.axis_label=None
p.axis.visible=False
p.grid.grid_line_color = None

show(p)

screen shot 2018-06-05 at 11 31 30

@bryevdv bryevdv added status: ready and removed status: WIP labels Jun 5, 2018

@bryevdv bryevdv requested a review from mattpap Jun 5, 2018

result[0] = 0
else
result[0] = col[0]
debugger

This comment has been minimized.

@mattpap

mattpap Jun 5, 2018

Contributor

^^^

This comment has been minimized.

@bryevdv

bryevdv Jun 5, 2018

Author Member

We should really just make a code quality check that looks for this

This comment has been minimized.

@mattpap

mattpap Jun 5, 2018

Contributor

Sure, there's a built-in rule for this (https://palantir.github.io/tslint/rules/no-debugger/).

This comment has been minimized.

@bryevdv

bryevdv Jun 5, 2018

Author Member

fixed in c591751

This comment has been minimized.

@mattpap

mattpap Jun 5, 2018

Contributor

If your are at fixing things, you can add "no-debugger": true to bokehjs/tslint.json.

This comment has been minimized.

@mattpap

mattpap Jun 5, 2018

Contributor

It's a part of codebase job on travis ci. Otherwise you have to run node make tslint. Perhaps we could make it part of quality tests at some point.

This comment has been minimized.

@mattpap

mattpap Jun 5, 2018

Contributor

but it doesn't currently actually fail when there is a violation:

It was supposed to get --emit-error, the same as scripts:compile and test:compile have. Must have forgotten about this or maybe there is a regression since then new build is in place.

This comment has been minimized.

@bryevdv

bryevdv Jun 5, 2018

Author Member

OK I added it, tho as I said it's not clear to me that a linter failure results in a test failure since the exit always seems to be 0

This comment has been minimized.

@bryevdv

bryevdv Jun 5, 2018

Author Member

I tried adding --emit-error locally, that still did not result in a nonzero exit code when a lint violation was present

This comment has been minimized.

@mattpap

mattpap Jun 5, 2018

Contributor

In fact this was done on purpose, because initially tslint reported a lot of errors and it was tedious dealing with this. Now all were resolved, so failing tslint build task on error should be implemented. I will do it with the next round of build improvements.

source = ColumnDataSource(data=dict(foo=[1, 2, 3, 4]))
CumSum('foo')

This comment has been minimized.

@philippjfr

philippjfr Jun 5, 2018

Contributor

Do Expressions actually allow positional arguments? Might be the case but maybe you confused it with the cumsum helper function signature?

This comment has been minimized.

@mattpap

mattpap Jun 5, 2018

Contributor

No, this code is incorrect.

This comment has been minimized.

@mattpap

mattpap Jun 5, 2018

Contributor

Also, it would be good to use actual Python shell syntax (>>>).

This comment has been minimized.

@bryevdv

bryevdv Jun 5, 2018

Author Member

fixed in 7f2a0dc

if (this.include_zero)
result[0] = 0
else
result[0] = col[0]

This comment has been minimized.

@mattpap

mattpap Jun 5, 2018

Contributor

This could have been reduced to one line as on the line before.

This comment has been minimized.

@bryevdv

bryevdv Jun 5, 2018

Author Member

fixed in 0c3eaa9

@bryevdv bryevdv merged commit 28a7f8c into master Jun 5, 2018

2 checks passed

Better Code Hub ✅ Better Code Hub approves this code
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bryevdv bryevdv deleted the bryanv/cumsum_transform branch Jun 5, 2018

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