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
Merged

Added span parameter for datashade operation #1508

merged 1 commit into from Jun 4, 2017

Conversation

@philippjfr
Copy link
Member

@philippjfr 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?

@jbednar
Copy link
Member

@jbednar 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
Copy link
Contributor

@jlstevens 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
Copy link
Member Author

@philippjfr 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
Copy link
Member

@jbednar 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
Copy link
Member

@jbednar 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
Copy link
Member Author

@philippjfr 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
Copy link
Member Author

@philippjfr philippjfr commented Jun 2, 2017

Might go back to clims since it's shorter.

@jlstevens
Copy link
Contributor

@jlstevens 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
Copy link
Member

@jbednar 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
Copy link
Contributor

@jlstevens 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
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
@philippjfr
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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants