Skip to content
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

philippjfr
Copy link
Member

@philippjfr 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 type: bug Something isn't correct or isn't working label Feb 2, 2018
Copy link
Member

@jbednar jbednar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the problem I was having; thanks!

@philippjfr
Copy link
Member Author

Now added tests, ready to merge once passing.

@philippjfr
Copy link
Member Author

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
Copy link
Contributor

@jlstevens jlstevens Feb 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suppose I see what you're getting at.

@jlstevens
Copy link
Contributor

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
Copy link
Member Author

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

@jlstevens
Copy link
Contributor

Tests are passing. Merging.

@jlstevens jlstevens merged commit 05f4545 into master Feb 5, 2018
@philippjfr philippjfr added this to the 1.9.3 milestone Feb 11, 2018
@philippjfr philippjfr deleted the datashader_categorical_fix branch February 11, 2018 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't correct or isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants