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

Improve handling of zero range datashader aggregations #2842

Merged
merged 9 commits into from Jun 29, 2018

Conversation

@philippjfr
Copy link
Member

@philippjfr philippjfr commented Jun 28, 2018

Ensures datashader aggregation on zero-range data is handled correctly. Previously there were some workarounds that did one of two things:

  1. If the range was zero-width then it was padded by an arbitrary factor of 0.5
  2. If the width or height were 0 then it would simply increase it to 1

Both were only workarounds and led to unexpected behavior so the operations now return Images with zero sized arrays. This is also related to holoviz/datashader#612 but doesn't require it since the operations are doing all the guarding against the bad cases that datashader doesn't handle correctly.

  • Fixes #2817
  • Adds unit tests
@jbednar
Copy link
Member

@jbednar jbednar commented Jun 28, 2018

Looks good, thanks.

@jbednar
Copy link
Member

@jbednar jbednar commented Jun 28, 2018

New changes look good too.

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Jun 28, 2018

Ready to review.

@jbednar
Copy link
Member

@jbednar jbednar commented Jun 28, 2018

Looks good to me, but would be helpful if @jlstevens looks it over too.

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Jun 28, 2018

One arbitrary thing I've done here is set the density for zero sized dimensions to 1, technically the density should be np.inf or zero but that causes all kinds of trouble.

@@ -468,6 +458,27 @@ def _process(self, element, key=None):
vdims = Dimension('Count')
params['vdims'] = vdims

if x is None or y is None or width == 0 or height == 0:
xarray = xr.DataArray(np.full((height, width), np.NaN, dtype=np.float32),

This comment has been minimized.

@jlstevens

jlstevens Jun 28, 2018
Contributor

Why does it have to be np.float32 in these two cases?

This comment has been minimized.

@philippjfr

philippjfr Jun 28, 2018
Author Member

No good reason, pointless optimization. Can take it out.

This comment has been minimized.

@jlstevens

jlstevens Jun 28, 2018
Contributor

I wouldn't specify a precision unless there is a good reason.

@jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Jun 29, 2018

Looks good to me and the tests are passing. Merging.

@jlstevens jlstevens merged commit 57cf92c into master Jun 29, 2018
4 checks passed
4 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 83.201%
Details
@philippjfr
s3-reference-data-cache Test data is cached.
Details
@philippjfr philippjfr deleted the datashade_zero_sized branch Jul 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants