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 span parameter for datashade operation #1508

Merged
merged 1 commit into from Jun 4, 2017

Conversation

Projects
None yet
3 participants
@philippjfr
Copy link
Contributor

philippjfr commented Jun 1, 2017

Not sure span is the best name for this parameter but I just used the datashader argument name for now. Maybe clims or color_range would be better?

@philippjfr philippjfr added the feature label Jun 1, 2017

@jbednar

This comment has been minimized.

Copy link
Contributor

jbednar commented Jun 1, 2017

I definitely don't like the name span. Is clims already in use for this purpose in HoloViews? If not it's more like a value_range.

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Jun 1, 2017

I don't think clims would clash in this case and it is clearer/more intuitive than span. Seems like the issue is that this isn't what this argument is called in datashader.

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Jun 1, 2017

Seems like the issue is that this isn't what this argument is called in datashader.

We already changed a bunch of the argument names because they're not very intuitive.

@jbednar

This comment has been minimized.

Copy link
Contributor

jbednar commented Jun 1, 2017

The datashader API needs some tidying up before I'm ready to call it 1.0, so don't worry about what it's called in datashader. I think HV should be supplying whatever dimension range it normally uses for non-datashader plots, though it could be convenient to provide an argument to shade that makes it simple to override that value when needed.

@jbednar

This comment has been minimized.

Copy link
Contributor

jbednar commented Jun 1, 2017

@philippjfr, can you please rename span to whatever the best match to HoloViews usage is, and then make it respect the dimension's range if not overridden here?

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Jun 1, 2017

@philippjfr, can you please rename span to whatever the best match to HoloViews usage is, and then make it respect the dimension's range if not overridden here?

Was going to go with value_range and already have those changes applied locally. Will push in a minute.

@philippjfr philippjfr force-pushed the datashade_span branch from a4fcad1 to cc32e0c Jun 2, 2017

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Jun 2, 2017

Might go back to clims since it's shorter.

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Jun 2, 2017

I also think clims is better: this operation outputs and RGB so there is color mapping and from what I understand this parameter is really about controlling colors.

@philippjfr philippjfr force-pushed the datashade_span branch from cc32e0c to 3a41767 Jun 2, 2017

@jbednar

This comment has been minimized.

Copy link
Contributor

jbednar commented Jun 2, 2017

clims is fine by me. It's an edge case; the actual parameter has nothing to do with colors, and applies to the value rather than to any colors, but it has no effect on anything but colors, in the end.

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Jun 4, 2017

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

@jlstevens jlstevens merged commit 1e06adb into master Jun 4, 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 decreased (-0.02%) to 78.91%
Details
s3-reference-data-cache Tests passing no test data changes required.
Details

@jbednar jbednar deleted the datashade_span branch Jun 5, 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.