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

Added datashader dynspread operation #1125

Merged
merged 2 commits into from Feb 14, 2017

Conversation

Projects
None yet
2 participants
@philippjfr
Copy link
Contributor

philippjfr commented Feb 11, 2017

As the title says this adds a dynspread operation which implements dynamic pixel spreading on RGB types. It works on all Image, RGB and GridImage types.

@philippjfr philippjfr added the feature label Feb 11, 2017

@philippjfr philippjfr force-pushed the datashader_dynspread branch from f609421 to ea914c5 Feb 11, 2017

pixels using a specified compositing operator. This can be useful
to make sparse plots more visible. Dynamic spreading determines
how many pixels to spread based on a density heuristic.
"""

This comment has been minimized.

@jbednar

jbednar Feb 11, 2017

Contributor

Maybe point to this section of the datashader manual, to pass the buck for having to explain it in any more detail? (Not that the datashader site necessarily does, just that if someone wants more explanation, that's the place it should be provided, not in holoviews.)

max_px = param.Integer(default=2, doc="""
Maximum number of pixels to spread on all sides.
""")

This comment has been minimized.

@jbednar

jbednar Feb 11, 2017

Contributor

Is this the HoloViews docstring style? It's up to you, but personally I much prefer to have it tighter:

max_px = param.Integer(default=2, doc="""
    Maximum number of pixels to spread on all sides.""")

This comment has been minimized.

@philippjfr

philippjfr Feb 11, 2017

Author Contributor

You're right, but we generally indent the docstring by four spaces.

This comment has been minimized.

@jbednar

jbednar Feb 11, 2017

Contributor

Oops; that's just from having a proportional font in Github's editing box. Fixed; I too always use 4.

@jbednar

This comment has been minimized.

Copy link
Contributor

jbednar commented Feb 11, 2017

Looks good, thanks!


threshold = param.Number(default=0.8, doc="""
A tuning parameter in the range [0, 1]. Indicates the minimum
value for the density heuristic.""")

This comment has been minimized.

@jbednar

jbednar Feb 12, 2017

Contributor

The important thing to convey here is the directionality -- does a higher value mean more spreading, or less?

This comment has been minimized.

@philippjfr

philippjfr Feb 12, 2017

Author Contributor

I just copied the datashader docstring so you presumably update it there as well.

This comment has been minimized.

@jbednar

jbednar Feb 13, 2017

Contributor

Right; the original isn't clear either. I don't have it running here in a handy way to test it, but if you do, please check which way it works, update the docstring, and then I'll merge and update the original docstring.

This comment has been minimized.

@jbednar

jbednar Feb 13, 2017

Contributor

How about:

threshold = param.Number(default=0.8, bounds=(0,1), doc="""
    When spreading, determines how far to spread.
    Spreading starts at 1 pixel, and stops when the fraction
    of adjacent non-empty pixels reaches this threshold.  
    Higher values give more spreading, up to the max_px 
    allowed.""")

This comment has been minimized.

@jbednar

jbednar Feb 13, 2017

Contributor

I've updated the docstring in the datashader source to say the above, now.

This comment has been minimized.

@philippjfr

philippjfr Feb 13, 2017

Author Contributor

Great will make the fix shortly.

pixels.""")

max_px = param.Integer(default=2, doc="""
Maximum number of pixels to spread on all sides.""")

This comment has been minimized.

@jbednar

jbednar Feb 13, 2017

Contributor

Is there a reason this default value does not match datashader's default value of 3? I think that could be confusing.

This comment has been minimized.

@philippjfr

philippjfr Feb 13, 2017

Author Contributor

Agreed, I copied from one of my examples.

@philippjfr philippjfr force-pushed the datashader_dynspread branch from 9d9556b to aee83cd Feb 13, 2017

Philipp Rudiger added some commits Feb 11, 2017

@philippjfr philippjfr force-pushed the datashader_dynspread branch from aee83cd to 57f5f10 Feb 13, 2017

@jbednar jbednar merged commit aaebffa into master Feb 14, 2017

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 (+1.1%) to 78.172%
Details
s3-reference-data-cache Test data is cached.
Details

@jbednar jbednar deleted the datashader_dynspread branch Feb 14, 2017

@philippjfr philippjfr added this to the v1.7.0 milestone Feb 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.