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
Member

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

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

Member

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

Member

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

Member

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

This comment has been minimized.

@jbednar

jbednar Feb 11, 2017

Member

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.

Member

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

Member

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

Member

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

This comment has been minimized.

@jbednar

jbednar Feb 13, 2017

Member

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

Member

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

Member

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

This comment has been minimized.

@philippjfr

philippjfr Feb 13, 2017

Member

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

Member

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

Member

Agreed, I copied from one of my examples.

@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