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 hover tool behavior on Curve types #1092

Merged
merged 8 commits into from Jan 31, 2017

Conversation

Projects
None yet
3 participants
@philippjfr
Member

philippjfr commented Jan 31, 2017

Fixes #986. Unfortunately in batched mode the hover tool will show all the values in the curve array rather than the specific coordinate you are hovering over so I've disabled displaying curve dimensions when in batched mode.

hv.NdOverlay({i: hv.Curve((range(4), (4-i,i*2, i+3, i))) for i in range(10)}, kdims=['z'])

Batched:

screen shot 2017-01-31 at 3 20 38 am

Non-batched:

screen shot 2017-01-31 at 3 19 21 am

@philippjfr philippjfr added the bokeh label Jan 31, 2017

@jbednar

This comment has been minimized.

Member

jbednar commented Jan 31, 2017

Why is x repeated for the non-batched case?

I'm not sure how to parse 'Element: 4'; is that the 4th curve from the bottom? Could you maybe assign names or values to the curves in the example, so that we can see that the name/value of that curve will be shown (hopefully true?)?

Also, apart from the name/value of the curve, presumably what we really want to see is the values of all other dimensions not mapped to the screen, at that point of that curve. I guess that's not possible? Presumably that's shown in the non-batched case? Would be good to clarify that, and to put that in the docs.

@jbednar

This comment has been minimized.

Member

jbednar commented Jan 31, 2017

Ok, that looks good. Presumably the second x should be suppressed when it's the same as x and y, for the non-batched case?

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jan 31, 2017

Why is x repeated for the non-batched case?

I've fixed that and renamed the dimension the Curves are indexed by to 'z' in the example.

Also, apart from the name/value of the curve, presumably what we really want to see is the values of all other dimensions not mapped to the screen, at that point of that curve.

In the non-batched case that will work all dimensions associated which each curve sample will be shown. In the batched case we will unfortunately only see the NdOverlay dimensions and values.

@jbednar

This comment has been minimized.

Member

jbednar commented Jan 31, 2017

Sounds good, thanks! Worth explaining anywhere we explain batched=True. Not sure why one can't do a lookup of whatever data we want to show, knowing the coordinates, but I guess that would turn this into something needing streams, whereas you are talking about Bokeh's native JS-based hover support?

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jan 31, 2017

I guess that would turn this into something needing streams, whereas you are talking about Bokeh's native JS-based hover support?

Right, indeed I don't really know how to modify the hover labels even using dynamic callbacks yet. This is purely about default hover behavior.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jan 31, 2017

Ready to merge when tests pass.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jan 31, 2017

Unfortunately, the new test_curve_overlay_hover test seems to be failing on Travis...

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jan 31, 2017

Ah, both a python3 issue and outdated test, will push shortly.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jan 31, 2017

This was an actual bug in the hover code in python3, hover was broken in some cases.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jan 31, 2017

Good to have unit tests.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jan 31, 2017

Ready to merge again.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jan 31, 2017

Looks good. Merging.

@jlstevens jlstevens merged commit 79422b9 into master Jan 31, 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 (+0.02%) to 77.9%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr deleted the curve_hover branch Feb 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment