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

Add support for extra hovertool variables in a Bokeh QuadMesh with 2-D coordinates (with tests) #5638

Merged
merged 8 commits into from
Mar 7, 2023

Conversation

JRRudy1
Copy link
Contributor

@JRRudy1 JRRudy1 commented Mar 5, 2023

When creating a Bokeh QuadMesh in the current version of HoloViews, you can supply additional data variables that will appear in the hovertool; however, this is only works for rectilinear geometries where the given coordinate arrays are 1-dimensional. When 2-dimensional coordinate arrays are given, any extra data variables are silently discarded and their values are displayed as "???" in the generated hovertool.

This pull request implements a simple fix that extends this functionality to support both 1-D and 2-D coordinates, which will allows QuadMesh plots to be created for non-rectilinear geometries without giving up the ability to add additional variables to the hovertool.

Three new unit tests were also added to verify this change; one for testing that the existing functionality for 1-D coordinates is not affected, and the other two for testing the new functionality for 2-D coordinates.

See the screenshot below for a comparison of the hovertool behavior in a Jupyter notebook before and after implementing the fix:

irregular_quadmesh_before_after

These unit tests check that any extra vdims provided when instantiating a `QuadMesh` with 2-D coordinate arrays are correctly passed to the Bokeh `ColumnDataSource` for use in hover labels. This test fails in HoloViews 1.15.4, which supports extra vdims when using 1-D coordinates but not 2-D coordinates.
This unit tests checks that any extra vdims provided when instantiating a `QuadMesh` with 1-D coordinate arrays are correctly passed to the Bokeh `ColumnDataSource` for use in hover labels. This feature is fully supported and functioning properly in HoloViews 1.15.4 (so the test should pass), unlike for 2-D coordinates wherein any extra vdims are lost.
…t_data` to a new staticmethod `QuadMeshPlot._collect_hover_data`.

This change was made in preparation for implementing the equivalent functionality in "irregular" meshes with 2-D coordinate arrays, which will enable the inclusion of additional variables in the hovertool for non-rectilinear geometries. All tests still pass (except for the newly added tests for the feature being implemented).
…QuadMesh` plots from 2-D coordinate arrays.

This change allows for the inclusion of additional variables in the hovertool for non-rectilinear geometries, which has long been supported for simple geometries defined by 1-D coordinate arrays. This fix only required very minor changes to the `QuadMeshPlot.get_data` method, and no other functionality should be affected.

All tests now pass, including the 2 new ones added to test this functionality, as well as the 3rd new test added to ensure that the existing behavior for 1-D coordinates was not affected.
…ticmethod to explain why the need for transposing the arrays before flattening is reversed when creating a `QuadMesh` from 2-D coordinate arrays.
@codecov-commenter
Copy link

codecov-commenter commented Mar 5, 2023

Codecov Report

Merging #5638 (95592f2) into main (eeabab2) will decrease coverage by 3.90%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5638      +/-   ##
==========================================
- Coverage   88.25%   84.35%   -3.90%     
==========================================
  Files         302      302              
  Lines       62532    62557      +25     
==========================================
- Hits        55189    52773    -2416     
- Misses       7343     9784    +2441     
Impacted Files Coverage Δ
holoviews/plotting/bokeh/raster.py 81.66% <100.00%> (+10.05%) ⬆️
...oloviews/tests/plotting/bokeh/test_quadmeshplot.py 100.00% <100.00%> (ø)
holoviews/tests/operation/test_datashader.py 1.75% <0.00%> (-95.87%) ⬇️
holoviews/operation/datashader.py 0.70% <0.00%> (-83.09%) ⬇️
holoviews/core/data/spatialpandas.py 14.70% <0.00%> (-72.61%) ⬇️
holoviews/tests/core/data/test_spatialpandas.py 34.13% <0.00%> (-64.08%) ⬇️
holoviews/core/data/spatialpandas_dask.py 50.79% <0.00%> (-31.75%) ⬇️
holoviews/tests/core/test_decollation.py 78.12% <0.00%> (-21.88%) ⬇️
holoviews/tests/test_selection.py 86.51% <0.00%> (-11.19%) ⬇️
holoviews/tests/core/test_datasetproperty.py 93.72% <0.00%> (-5.59%) ⬇️
... and 27 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@JRRudy1
Copy link
Contributor Author

JRRudy1 commented Mar 5, 2023

It looks like the 3 failed checks are just a few tests getting skipped due to datashader being missing from the CI environment, but please let me know if I did something wrong. This is my first ever PR, and I am very open to advice from anyone reviewing it.

@hoxbro
Copy link
Member

hoxbro commented Mar 5, 2023

The failing tests on Python 3.7 are unrelated to your changes. So don't worry about those 🙂

I will take an in-depth look at this PR tomorrow.

@JRRudy1
Copy link
Contributor Author

JRRudy1 commented Mar 5, 2023

Glad to hear, thanks!

Copy link
Member

@hoxbro hoxbro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What an amazing first PR! 🥇

I have one small question.

holoviews/plotting/bokeh/raster.py Outdated Show resolved Hide resolved
…ollect_hover_data` method.

This involved converting the staticmethod to an instance method, changing the `transpose` argument to `irregular`, and then adding a line to derive `transpose` from `invert_axes` and `irregular`.

This change was made to increase clarity by putting the unintuitive step closer to where it is used, and right next to the docstring explaining it.
@hoxbro hoxbro merged commit 8d3a745 into holoviz:main Mar 7, 2023
@JRRudy1 JRRudy1 deleted the bokeh-quadmesh-extra-vdims branch March 7, 2023 20:48
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants