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

Implement multi-yaxis support #5621

Merged
merged 64 commits into from
Jul 21, 2023
Merged

Implement multi-yaxis support #5621

merged 64 commits into from
Jul 21, 2023

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Feb 10, 2023

This adds multi-yaxis support to the Bokeh backend.

Screen Shot 2023-02-10 at 16 21 11

ToDo

  • Clean up utilities
  • Add autorange support for multi-axes
  • Fix linking of shared axes
  • Implement axis labels correctly
  • Handle correct logx/logy mappings
  • Correctly determine categorical axis creation per axis
  • Clean up _get_factors
  • Handle dynamic axis creation when DynamicMap OverlayPlot adds new elements

@philippjfr philippjfr changed the title Implement multi-axes support Implement multi-yaxis support Feb 10, 2023
Copy link
Member

@jbednar jbednar left a comment

Choose a reason for hiding this comment

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

Very cool! How does a user configure this behavior? I think we should introduce it as an option at first since it's a change in behavior that some people may previously have been relying on (e.g. by defining dimensions only on the first plot in an overlay, rather than (correctly) defining them on all elements of the overlay).

holoviews/plotting/bokeh/element.py Outdated Show resolved Hide resolved
@philippjfr
Copy link
Member Author

@jlstevens and I discussed this and I am still strongly of the opinion that we should never enable this by default. Right now you enable this with the multi_y option and I think we should keep that. What we discussed as an alternative is that if you have multiple clashing vdims we will simply put all of the vdim labels on the axis (probably separated by a pipe).

My concern with enabling it by default continues to be that it is likely to be much more annoying than useful. Particularly when creating plots directly from DataFrames or xarray you will often overlay things with different column/variable names and a slight mismatch shouldn't result in multiple axes.

@jbednar
Copy link
Member

jbednar commented Feb 10, 2023

Thanks; I didn't spot multi_y in the PNG. Sounds good.

@jlstevens
Copy link
Contributor

jlstevens commented Feb 10, 2023

I think I am happy with multi_y as an additional parameter: it gates the functionality, preserving backwards compatibility for now. Then, to make things more convenient again (but backwards incompatible) for HoloViews 2.0 we can simply set the default to True.

@philippjfr
Copy link
Member Author

As I said above and suggested to you, I vote strongly never to enable it by default.

@jlstevens
Copy link
Contributor

jlstevens commented Feb 10, 2023

As I said above and suggested to you, I vote strongly never to enable it by default.

As I have no strong opinion this is also fine by me! That said, I also think other people may disagree and that this is something to discuss again when the time comes. Specifying an additional flag is more cumbersome and inconvenient though in this case, I could easily be convinced being explicit is the overriding factor.

@jbednar
Copy link
Member

jbednar commented Feb 10, 2023

We'll see! I'll try setting it on for my workflows, and see what problems it causes in practice. Meanwhile, no need to decide about future possible changes to defaults.

@jlstevens jlstevens added this to the 1.16.0 milestone Feb 15, 2023
@orozcojd
Copy link

Is this expected to work with Datashaded plots?

@philippjfr
Copy link
Member Author

Yes, absolutely.

@GeoVizNow
Copy link
Contributor

Hi, I am really looking forward to having support for multiple vdim axes. Was wondering if there are plans to include some option to configure an holoviews.Area element with multiple axes such that colors are swapping when y1 and y2 referenced to different axes are crossing. i.e. to make the below example in matplotlib a oneline with bokeh (or other backends) in holoviews.

from matplotlib import pyplot as plt
import numpy as np

xs = np.linspace(0, np.pi*4, 100)
ys = np.sin(xs)
ys2 = (np.sin(xs) * .01) + .2

plt.plot(xs, ys, color='black', zorder=20)
plt.plot(xs, ys2, color='blue', zorder=20)
plt.grid(visible=True)
plt.yticks(ticks=[-1, 0, 1])
plt.fill_between(x=xs, y1=ys, y2=ys2, where=ys >= ys2, zorder=10)
plt.fill_between(x=xs, y1=ys, y2=ys2, where=ys <= ys2, color='green', zorder=10)

image

@jbednar
Copy link
Member

jbednar commented Jun 12, 2023

Was wondering if there are plans to include some option to configure an holoviews.Area element with multiple axes such that colors are swapping when y1 and y2 referenced to different axes are crossing.

@GeoVizNow, that sounds unrelated to having multiple y axes, and seems like it should be doable already by overlaying multiple hv.Area plots cropped in comparison to a reference curve. Seems like a discourse.holoviz.org topic to propose and see if someone can fill in an example.

@codecov-commenter
Copy link

codecov-commenter commented Jun 30, 2023

Codecov Report

Merging #5621 (e319a5a) into main (d48950a) will increase coverage by 0.01%.
The diff coverage is 92.19%.

@@            Coverage Diff             @@
##             main    #5621      +/-   ##
==========================================
+ Coverage   88.19%   88.20%   +0.01%     
==========================================
  Files         307      308       +1     
  Lines       63305    63628     +323     
==========================================
+ Hits        55829    56125     +296     
- Misses       7476     7503      +27     
Flag Coverage Δ
ui-tests 22.30% <16.15%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
holoviews/plotting/bokeh/util.py 66.89% <69.76%> (+0.08%) ⬆️
holoviews/plotting/plot.py 91.71% <88.46%> (-0.20%) ⬇️
holoviews/plotting/bokeh/element.py 88.17% <90.69%> (+0.21%) ⬆️
holoviews/tests/plotting/bokeh/test_multiaxis.py 98.28% <98.28%> (ø)
holoviews/plotting/bokeh/annotation.py 85.14% <100.00%> (ø)
holoviews/plotting/bokeh/chart.py 88.18% <100.00%> (-0.17%) ⬇️
holoviews/plotting/bokeh/graphs.py 91.32% <100.00%> (-0.18%) ⬇️
holoviews/plotting/bokeh/heatmap.py 91.76% <100.00%> (ø)
holoviews/plotting/bokeh/hex_tiles.py 94.73% <100.00%> (ø)
holoviews/plotting/bokeh/sankey.py 80.47% <100.00%> (ø)
... and 7 more

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

@jlstevens
Copy link
Contributor

Bokeh 3 tests are now green! :-)

However, something since 2025b5d has broken Bokeh 2 badly (which now hangs. My suspicion is that it could be due to the Field import but I could be wrong...

@jlstevens
Copy link
Contributor

Adding a note as a reminder that we will need a proper fix for the new dask release and to unpin in setup.py!

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.

@philippjfr and I have reviewed your code and left some comments and suggestions for you.

Something to note is we haven't reviewed the Javascript code in callback = CustomJS(code=f"""

One last thing is, please doublecheck that all get_extents has the same signature.

examples/user_guide/Customizing_Plots.ipynb Outdated Show resolved Hide resolved
holoviews/plotting/bokeh/element.py Outdated Show resolved Hide resolved
holoviews/plotting/bokeh/element.py Outdated Show resolved Hide resolved
holoviews/plotting/bokeh/element.py Outdated Show resolved Hide resolved
holoviews/plotting/bokeh/element.py Show resolved Hide resolved
holoviews/plotting/bokeh/util.py Outdated Show resolved Hide resolved
holoviews/plotting/plot.py Outdated Show resolved Hide resolved
holoviews/plotting/plot.py Show resolved Hide resolved
holoviews/plotting/plot.py Outdated Show resolved Hide resolved
holoviews/plotting/plot.py Outdated Show resolved Hide resolved
@hoxbro
Copy link
Member

hoxbro commented Jul 21, 2023

As discussed with @philippjfr, this PR is in a good state and can be merged, and then I will tag a new dev version.

Thank you for all your work here, @jlstevens! I know how you have loved every single second of bringing this fantastic functionality to life!

@hoxbro hoxbro merged commit 2d7e1cc into main Jul 21, 2023
@hoxbro hoxbro deleted the multi_axes branch July 21, 2023 18:04
@jlstevens
Copy link
Contributor

I know how you have loved every single second of bringing this fantastic functionality to life!

Of course! :-)

@GeoVizNow
Copy link
Contributor

Thank you for working up this great new feature!

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.

7 participants