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.

@philippjfr philippjfr added the type: bug Something isn't correct or isn't working label Jun 28, 2018
@jbednar
Copy link
Member

jbednar commented Jun 28, 2018

Looks good, thanks.

@jbednar
Copy link
Member

jbednar commented Jun 28, 2018

New changes look good too.

@philippjfr
Copy link
Member Author

Ready to review.

@jbednar
Copy link
Member

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@jlstevens
Copy link
Contributor

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

@jlstevens jlstevens merged commit 57cf92c into master Jun 29, 2018
@philippjfr philippjfr deleted the datashade_zero_sized branch July 4, 2018 11:12
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: bug Something isn't correct or isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants