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

Fixed bug computing categorical datashader aggregates #2295

Merged
merged 6 commits into from Feb 5, 2018

Conversation

Projects
None yet
3 participants
@philippjfr
Member

philippjfr commented Feb 2, 2018

Recently I added some validation to xarray Datasets that disallows coords (kdims) and data variables (vdims) with the same name, which was apparently being used by the datashader aggregate operation for categorical aggregates. This handles them correctly.

  • Add tests for categorical aggregates

@philippjfr philippjfr added the bug label Feb 2, 2018

@jbednar

jbednar approved these changes Feb 2, 2018

This fixes the problem I was having; thanks!

@philippjfr

This comment has been minimized.

Member

philippjfr commented Feb 3, 2018

Now added tests, ready to merge once passing.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Feb 3, 2018

I've also pushed a fix for #2299 to this PR and added a unit test.

if isinstance(agg_fn, ds.count_cat):
name = '%s Count' % agg_fn.column
vdims = [dims[0](column)]
name = '%s Count' % column

This comment has been minimized.

@jlstevens

jlstevens Feb 5, 2018

Member

name = ('%s Count' % column) if isinstance(agg_fn, ds.count_cat) else column?

Personally, I don't like it when I see simple variable reassignments e.g x=y used in this way. Up to you, I won't hold the merge up because of this.

This comment has been minimized.

@philippjfr

philippjfr Feb 5, 2018

Member

There is more to the if/else block than just this single line.

This comment has been minimized.

@philippjfr

philippjfr Feb 5, 2018

Member

Suppose I see what you're getting at.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Feb 5, 2018

Happy to merge once you reply to my comment. I won't hold up the merge if you don't want to make the suggested change.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Feb 5, 2018

Made the change, ready to merge once tests pass again.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Feb 5, 2018

Tests are passing. Merging.

@jlstevens jlstevens merged commit 05f4545 into master Feb 5, 2018

3 of 4 checks passed

continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 81.13%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr added this to the 1.9.3 milestone Feb 11, 2018

@philippjfr philippjfr deleted the datashader_categorical_fix branch Feb 11, 2018

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