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

Interactive layout fixes #136

Merged
merged 8 commits into from
Nov 6, 2018
Merged

Interactive layout fixes #136

merged 8 commits into from
Nov 6, 2018

Conversation

philippjfr
Copy link
Member

The interactive layout was previously self-referential which causes issues when trying to display the plot/display component on its own. Additionally this ensures that a small fixes to assign a minimal height when no height is specified for the last item in a Column.

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 good. What does the representation of an interactive object look like now?

panel/layout.py Show resolved Hide resolved
@@ -118,8 +138,7 @@ def _get_model(self, doc, root=None, parent=None, comm=None):
objects = self._get_objects(model, [], doc, root, comm)

# HACK ALERT: Insert Spacer if last item in Column has no height
if (isinstance(self, Column) and objects and not isinstance(objects[-1], (BkWidgetBox, BkBox))
and getattr(objects[-1], 'height', False) is None):
if (isinstance(self, Column) and objects and no_height(objects[-1])):
objects.append(BkSpacer(height=50))
Copy link
Member

Choose a reason for hiding this comment

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

Add a parameter default_height? Could be useful e.g. for the report mode dashboards...

Copy link
Member Author

@philippjfr philippjfr Nov 6, 2018

Choose a reason for hiding this comment

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

Where would the parameter live, on Column? I'd be happy to add it but maybe in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

Well, height=50 is used here on Panel, so presumably on Panel, so that it can be height=self.default_height

Copy link
Member Author

Choose a reason for hiding this comment

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

It's specifically behind an isinstance check for Column to avoid having to duplicate the whole method there.

Copy link
Member Author

Choose a reason for hiding this comment

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

That said I could add an equivalent check for Row:

(isinstance(self, Row) and objects and not any(has_height(o) for o in objects))

Copy link
Member Author

Choose a reason for hiding this comment

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

Merging this, but I can follow up on this tomorrow.

panel/layout.py Show resolved Hide resolved
@jbednar
Copy link
Member

jbednar commented Nov 5, 2018

I'm seeing errors on this PR with "ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()" with https://anaconda.org/jbednar/datashadercliffordinteract/notebook

@philippjfr
Copy link
Member Author

Not sure how that could be caused by this PR.

@jbednar
Copy link
Member

jbednar commented Nov 5, 2018

Not sure either, just that it works with master (v0.2.0a4-1-ge98e151) and not with this branch (v0.2.0a4-6-g40bc2ba).

@philippjfr
Copy link
Member Author

Looks like an issue in the repr.

@codecov-io
Copy link

codecov-io commented Nov 6, 2018

Codecov Report

Merging #136 into master will increase coverage by 0.04%.
The diff coverage is 98.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #136      +/-   ##
==========================================
+ Coverage   89.88%   89.92%   +0.04%     
==========================================
  Files          28       28              
  Lines        4181     4209      +28     
==========================================
+ Hits         3758     3785      +27     
- Misses        423      424       +1
Impacted Files Coverage Δ
panel/util.py 80.64% <100%> (+1.33%) ⬆️
panel/tests/test_interact.py 100% <100%> (ø) ⬆️
panel/param.py 90.75% <100%> (+0.21%) ⬆️
panel/interact.py 71.42% <100%> (-0.28%) ⬇️
panel/pane.py 81.84% <100%> (ø) ⬆️
panel/viewable.py 69.26% <100%> (-0.28%) ⬇️
panel/layout.py 95.19% <92.3%> (+0.21%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fce1a02...ec61d5e. Read the comment docs.

@philippjfr philippjfr merged commit c6039bb into master Nov 6, 2018
@jbednar jbednar deleted the interact_fixes branch November 6, 2018 19:16
@jbednar
Copy link
Member

jbednar commented Nov 6, 2018

This change really improved the usability of what's returned by interact(), and I've updated https://anaconda.org/jbednar/datashadercliffordinteract/notebook to reflect that. Now, the repr is something even more clear and clearly usable:

>>> i = pn.interact(clifford_plot, a=(-2,2), b=(-2,2), c=(-2,2), d=(-2,2), 
              n=(1,20000000), colormap=ps, panel_layout=pn.Row)
>>> print(i)


Column
    [0] WidgetBox
        [0] FloatSlider(end=2, name='a', start=-2, value=1.7)
        [1] FloatSlider(end=2, name='b', start=-2, value=1.7)
        [2] FloatSlider(end=2, name='c', start=-2, value=0.6)
        [3] FloatSlider(end=2, name='d', start=-2, value=1.2)
        [4] IntSlider(end=20000000, name='n', start=1, value=2000000)
        [5] Select(name='colormap', options={'bgy': ['#fff123', ...}, value=['#b3fef5', '#b0fef5', ...])
    [1] Row
        [0] PNG(Image, name='interactive00026')

So it's now obvious that one can do pn.Column(i[0][5], i[1][0]) to get a column with just the Select widget above the PNG image, with no opaque interact objects. Very nice!

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.

3 participants