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

Improve hover tool behavior on Curve types #1092

Merged
merged 8 commits into from Jan 31, 2017
Merged

Improve hover tool behavior on Curve types #1092

merged 8 commits into from Jan 31, 2017

Conversation

@philippjfr
Copy link
Member

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

@jbednar
Copy link
Member

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

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

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

@jbednar 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 philippjfr force-pushed the curve_hover branch from 06a54b9 to 2662ce0 Jan 31, 2017
@philippjfr
Copy link
Member Author

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

@philippjfr philippjfr commented Jan 31, 2017

Ready to merge when tests pass.

@jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Jan 31, 2017

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

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Jan 31, 2017

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

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Jan 31, 2017

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

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Jan 31, 2017

Good to have unit tests.

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Jan 31, 2017

Ready to merge again.

@jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Jan 31, 2017

Looks good. Merging.

@jlstevens jlstevens merged commit 79422b9 into master Jan 31, 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 increased (+0.02%) to 77.9%
Details
@philippjfr
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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants