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 fancy_layout option to HoloViews pane #543

Merged
merged 9 commits into from Aug 5, 2019
Merged

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Jul 21, 2019

The current HoloViews pane generates a simple layout of the plot and the set of widgets generated from that plot. This makes it easy to compose these two components in different ways but it doesn't match the look that the old-style HoloViews widgets use. Since we want to swap out the widgets in HoloViews with as little disruption as possible this PR adds a fancy_layout option which tries to match the look and feel of the original widget implementation.

As a simple example here is the output of a HoloMap with this option enabled:

Screen Shot 2019-07-21 at 1 23 32 PM

@philippjfr philippjfr force-pushed the philippjfr/holoviews_fancy branch from acff765 to a58efd6 Compare Aug 5, 2019
@philippjfr philippjfr merged commit 6ef5b51 into master Aug 5, 2019
2 of 3 checks passed
@jbednar
Copy link
Member

@jbednar jbednar commented Aug 5, 2019

I se this is merged already, but I'm wondering whether fancy is the right term for this arrangement. Fancy makes it sound like it's better than the default, but really it's just different, as far as I can see. Could we use a more neutral word here, like shaded_widgets?

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Aug 5, 2019

Happy to change it, about to open another PR to improve the scrubber layout anyway, but -1 on shaded. Open to other suggestions.

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Aug 5, 2019

The differences are:

  1. The widgets are inside a widget box (which is shaded and gray)
  2. The plot and widgets are horizontally centered and the margins are responsive
  3. The widget box is vertically centered

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Aug 5, 2019

Also for context, the "fancy" bit is borrowed from matplotlib's fancybox option on legends: https://matplotlib.org/gallery/recipes/transparent_legends.html

@jbednar
Copy link
Member

@jbednar jbednar commented Aug 5, 2019

I don't think it shares much in common with the mpl fancybox option, which isn't grey or offset like that. Maybe center_shaded? I'm just looking for something more literal and with no valence. But it's only a slight preference, so if there's no literal name you are happy with, then I guess just keep it fancy.

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Aug 5, 2019

Cc: @jsignell @jlstevens @jonmmease Any opinions or suggestions?

@jsignell
Copy link
Member

@jsignell jsignell commented Aug 5, 2019

What about original_layout or classic_layout

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Aug 5, 2019

I like classic_layout.

@jbednar
Copy link
Member

@jbednar jbednar commented Aug 5, 2019

classic_layout is fine if it's not something we plan to support forever or expect new users to consider using.

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Aug 5, 2019

classic_layout is fine if it's not something we plan to support forever or expect new users to consider using.

We do plan to support it forever, it's the default layout HoloViews will use to render its widgets. Panel users should generally use the default layout so they can compose it themselves more easily.

@jonmmease
Copy link
Member

@jonmmease jonmmease commented Aug 5, 2019

I don't have strong feeling on the name itself, but should we consider this to be an enumeration rather than a boolean? Something like layout_style='holoviews', and then leave open the possibility of other styles in the future.

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Aug 5, 2019

Something like layout_style='holoviews', and then leave open the possibility of other styles in the future.

Not a bad idea either.

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Aug 5, 2019

Currently the options would be 'simple' and 'classic' or 'holoviews'.

@jonmmease
Copy link
Member

@jonmmease jonmmease commented Aug 5, 2019

As a disclaimer, I haven't looked at it closely enough to have an opinion on whether we would want to have more options like this in the future. I just know I've often come to regret boolean arguments 🙂

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Aug 5, 2019

As a disclaimer, I haven't looked at it closely enough to have an opinion on whether we would want to have more options like this in the future

I would certainly have to refactor the code and come up with some better representation to express custom layouts because the code is already getting unwieldy with two options but I think more flexibility would be a good thing. People certainly have asked for more flexibility in HoloViews and while they can already achieve that if they use Panel directly and lay things out themselves I don't think it would hurt to include a few more canned layouts they can pick from.

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Aug 5, 2019

If no one objects I'm going with:

layout_style = param.ObjectSelector(default='simple', objects=['simple', 'classic'], doc="""
    The default layout of the plot and widgets. The default is a 'simple' layout to make
    it easy to compose the plot and the widgets manually.""")

@jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Aug 5, 2019

The last suggestion above looks good to me in terms of a layout_style parameter. I think 'simple' is clear enough as a name (the current panel behavior afaik) but 'classic' isn't too informative as to what that really means.

I am not a fan of 'holoviews' as a value and 'adjoint' is probably confusing given holoviews uses that word as well. I'm wouldn't let 'classic' block this PR but I would like to try and think of something else if possible (tricky!).

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Aug 5, 2019

Could call it centered I guess but there could be a number of centered arrangements. One problem is that it also interacts with the widget_type parameter. Maybe we could add center_right, center_left, center_bottom, center_top, left, right, bottom and top options instead. HoloViews would use center_right for regular widget and center_bottom for the scrubber.

@jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Aug 5, 2019

Those options make sense to me and generalize panel nicely. Of course it is also more work to do but this would be my preferred approach in terms of offering a general and intuitive API...

@jbednar
Copy link
Member

@jbednar jbednar commented Aug 5, 2019

I was reading the recent discussion starting from the top and independently thought of center_right before reading @philippjfr's comment, so I vote for that -- that way people can declare an appropriate default mapping to a layout that they prefer in general, and of course then they can always repack things manually using Panel if they wish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants