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

Replace HoloViews widgets with Panel #3836

Merged
merged 43 commits into from
Aug 15, 2019
Merged

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Jul 19, 2019

Replacing HoloViews widgets with Panel widgets.

Requires holoviz/panel#543 to match the previous styling.

Closes #3868
Closes #3212
Closes #84
Closes #3027

@jonmmease
Copy link
Collaborator

If I merge #3837 into this branch, and use the panel version from holoviz/panel#529, I'm able to get a plotly HoloMap to update with the slider 🎉

import numpy as np
import holoviews as hv
import panel as pn
hv.extension("plotly")

frequencies = [0.5, 0.75, 1.0, 1.25]

def sine_curve(phase, freq):
    xvals = [0.1* i for i in range(100)]
    return hv.Curve((xvals, [np.sin(phase+freq*x) for x in xvals]))

curve_dict = {f:sine_curve(0,f) for f in frequencies}

hmap = hv.HoloMap(curve_dict, kdims='frequency')
hmap

Screen Shot 2019-07-24 at 8 01 22 AM

I do see an error in the JS console each time the slider is moved.

Screen Shot 2019-07-24 at 8 05 33 AM

But the plot does update successfully.

I was trying to get a better understanding of what the JS side is doing here, so I saved the HoloMap to a standalone HTML file with

hv.save(hmap, 'hmap_plotly.html')

But in the resulting HTML file the slider updates don't update the plot. This is also the case for the bokeh extension.

@philippjfr
Copy link
Member Author

I do see an error in the JS console each time the slider is moved.

That should be fixed in bokeh 1.3.0, it's some minor bug to do with the slider tooltip.

@philippjfr
Copy link
Member Author

I was trying to get a better understanding of what the JS side is doing here, so I saved the HoloMap to a standalone HTML file with

Still need to look at this, suspect save does not correctly use the panel embed features yet.

@jonmmease
Copy link
Collaborator

Do you think this will be a somewhat long-running branch? Would it make sense for me to PR the plotly updates into this branch?

@philippjfr
Copy link
Member Author

Do you think this will be a somewhat long-running branch? Would it make sense for me to PR the plotly updates into this branch?

I think that probably makes sense. Can't say quite how long this will take and keeping the two PRs in sync seems like a pain.

@jonmmease
Copy link
Collaborator

Sounds good. I'll make your suggested updates in the other branch, then rebase on this and open a new PR.

@jonmmease
Copy link
Collaborator

Digging into the save issue a little. The resulting Bokeh state model is empty. And I think it starts with the fact that panel.select(Widget) returns empty on the panel.pane.holoviews.HoloViews object that .save(embed=True) is called on.

https://github.com/pyviz/panel/blob/b274c45252c54091dab4caf38a2b4b9a31d75c4b/panel/io/embed.py#L105-L106

panel.widgets is empty, but panel.widgets_from_dimensions(panel.object) returns the DiscreteSlider instance.

@philippjfr
Copy link
Member Author

Fixed the widget saving issue.

@jonmmease
Copy link
Collaborator

capturing note by @philippjfr from #3851 (comment).

I think at least for the simple matplotlib case we should probably not require panel...

This got me thinking, rather than just doing this for matplotlib maybe we should have a mode that allows non-dynamic non-widget figures to be rendered by their respective backend libraries. In version 4, plotly.py has a pretty flexible rendering system and can display plots in a bunch of contexts that full-blown HoloViews can't (Colab, VSCode, etc.). And of course matplotlib works everywhere, and I believe bokeh's support is pretty wide as well.

@jbednar
Copy link
Member

jbednar commented Aug 5, 2019

I agree; sounds like a good baseline so that people can still use most HoloViews/hvPlot functionality anywhere.

@philippjfr
Copy link
Member Author

In version 4, plotly.py has a pretty flexible rendering system and can display plots in a bunch of contexts that full-blown HoloViews can't (Colab, VSCode, etc.).

Last I checked we can render in VSCode and nteract now. Colab is an issue because they iframe the cell outputs which means the extension has to be loaded for each individual cell. I'd be okay leaving that for a later PR and doing the simplest thing in this PR.

philippjfr and others added 9 commits August 14, 2019 02:30
* Use the Panel HoloViews model for all HTML format requests
(not only for the hv objects with widgets)

* Don't call `_figure_data` with 'html' format, handle these through Panel

* Remove fmt='html' logic from _figure_data methods.

`as_script` arg still supported to return static images as HTML fragments,
but since there is no associated JavaScript in this case the method no longer
returns a tuple when `as_script=True`.

* Initialize appropriate panel extensions in load_nb methods

Remove unneeded notebook initialization code

* Remove unneeded HTML/JS code and templates from Bokeh and Plotly renderers

* Fix plotly save to SVG encoding
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.

Looks fabulous! I only reviewed the new code, not what was deleted, but I don't think I'll miss any of the deleted bits. Upshot:

image

holoviews/plotting/plot.py Outdated Show resolved Hide resolved
holoviews/tests/plotting/bokeh/testrenderer.py Outdated Show resolved Hide resolved
@jlstevens
Copy link
Contributor

Looks great! Very happy to see a load of nasty code (including inline JS) go away...

I have two very minor suggestions relating to the code but otherwise I am happy to merge. I suppose my only other thought is it would be good to record the sort of (hopefully minor!) changes people might expect to see the CHANGELOG/release notes...

The rendered size as a percentage size""")

widget_location = param.ObjectSelector(default=None, allow_None=True, objects=[
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to have a shorted name than widget_location but I don't currently have any better suggestions...

@jlstevens
Copy link
Contributor

I won't wait for appveyor to go green as it is taking a while to start and all the other tests are green. I'm very happy to see 2000+ lines of old widget code removed and to let panel handle that side of HoloViews in a more maintainable and future proof way. Merging!

@jlstevens jlstevens merged commit 272391f into master Aug 15, 2019
@philippjfr philippjfr deleted the philippjfr/widgetsectomy branch September 20, 2019 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment