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 GridSpace axes in bokeh #1150

Merged
merged 9 commits into from Mar 6, 2017

Conversation

Projects
None yet
3 participants
@philippjfr
Member

philippjfr commented Feb 25, 2017

Creates outer dimension axes for GridSpaces, like we support in matplotlib:

screen shot 2017-02-25 at 2 21 52 pm

@philippjfr philippjfr added bokeh feature WIP and removed WIP labels Feb 25, 2017

@philippjfr

This comment has been minimized.

Member

philippjfr commented Feb 26, 2017

Small change to test data due to a small change in how grid plots are padded. Ready to merge now.

@philippjfr philippjfr requested a review from jlstevens Feb 26, 2017

@philippjfr

This comment has been minimized.

Member

philippjfr commented Feb 27, 2017

Merge this one last, after all the latest once since it will change the test data and then all other PRs will need rebasing.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Feb 27, 2017

All the other HoloViews PRs are now merged so feel free to rebase now and update the test data as appropriate.

kwargs['xaxis'] = 'bottom-bare'
kwargs['width'] = 150
kwargs['xaxis'] = None
kwargs['width'] = self.plot_size+50
if c != 0 and r == 0:

This comment has been minimized.

@jlstevens

jlstevens Feb 27, 2017

Member

Looks like 50 is a magic number...

This comment has been minimized.

@philippjfr

philippjfr Feb 27, 2017

Member

True, it's the offset used to account for the axis width. Could parameterize it I suppose.

This comment has been minimized.

@jbednar

jbednar Feb 27, 2017

Member

Would be much better to query it than parameterize it. Can it be calculated as outer dimension minus inner dimension, now that the inner dimensions are available in Bokeh JS?

This comment has been minimized.

@philippjfr

philippjfr Feb 27, 2017

Member

Can it be calculated as outer dimension minus inner dimension, now that the inner dimensions are available in Bokeh JS?

No bokehJS is too late to set this, if it's wrong the layout solver breaks and you never get an inner width.

if axis == 'x':
align = 'center'
# Adjust height to compensate for label rotation
height = int(50 + np.abs(np.sin(rotation)) * (nchars*8))

This comment has been minimized.

@jlstevens

jlstevens Feb 27, 2017

Member

I assume this is the same magic number 50. Even if it doesn't have to be a parameter, it should at least be a class attribute...

This comment has been minimized.

@philippjfr

philippjfr Feb 27, 2017

Member

Sure, might be good to parameterize it actually, if you have really long axis labels the layout solver could blow up I suppose and this would let you have some control.

This comment has been minimized.

@jlstevens

jlstevens Feb 27, 2017

Member

A class attribute seems like a step in the right direction for now. I wouldn't make it a parameter till we have a concrete, real world example of it being needed.

This comment has been minimized.

@jlstevens

jlstevens Feb 27, 2017

Member

What about the 8 in nchars*8? Where is the 8 from?

This comment has been minimized.

@philippjfr

philippjfr Feb 27, 2017

Member

Cheating a bit, it's the fontsize, which I actually don't let you change atm. Could allow changing it and scaling this size accordingly.

This comment has been minimized.

@jbednar

jbednar Feb 27, 2017

Member

Sounds very fragile.

This comment has been minimized.

@philippjfr

philippjfr Feb 27, 2017

Member

I'm actually surprised how well it works, but it will definitely get more brittle if I add support for fontsizes.

This comment has been minimized.

@philippjfr

philippjfr Feb 27, 2017

Member

It also only works because I'm explicitly formatting the ticks as strings, which makes sense in this case, but it is not a general approach because it will break for ticks computed on the frontend.

@@ -205,7 +205,7 @@ def test_get_size_grid_plot(self):
grid = GridSpace({(i, j): self.image1 for i in range(3) for j in range(3)})
plot = self.renderer.get_plot(grid)
w, h = self.renderer.get_size(plot)
self.assertEqual((w, h), (360, 360))
self.assertEqual((w, h), (418, 410))

This comment has been minimized.

@jlstevens

jlstevens Feb 27, 2017

Member

Interesting that it used to be perfectly square, but isn't anymore. Generally, square plots seems easier when considering layouts etc...

This comment has been minimized.

@philippjfr

philippjfr Feb 27, 2017

Member

Interesting that it used to be perfectly square, but isn't anymore.

Taking into account the fact that a vertical axis is wider than a horizontal one. Might be worth considering using this calculation more generally because I've found I can compute axes widths pretty accurately.

This comment has been minimized.

@philippjfr

philippjfr Feb 27, 2017

Member

(Probably not a good idea when a JS formatter is responsible for computing ticks).

@@ -125,7 +128,7 @@ def mpl_to_bokeh(properties):
return new_properties
def layout_padding(plots):
def layout_padding(plots, renderer):
"""
Temporary workaround to allow empty plots in a
row of a bokeh GridPlot type. Should be removed

This comment has been minimized.

@jlstevens

jlstevens Feb 27, 2017

Member

The PR reference in this docstring bokeh/bokeh#2891 looks like it was resolved to your satisfaction in June. This suggests either this function should be removed or the docstring updated.

This comment has been minimized.

@philippjfr

philippjfr Feb 27, 2017

Member

Yes, will update the docstring (and perhaps reopen the issue). It's not actually fixed.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Feb 28, 2017

While the approach isn't ideal it works robustly for a wide range of font sizes and grid sizes I've tried and GridSpaces are really useful particularly with the linked zooming that bokeh supports. Here's what the example from the containers tutorial looks like now rendered with bokeh:

screen shot 2017-02-28 at 2 19 45 am

@jbednar

This comment has been minimized.

Member

jbednar commented Feb 28, 2017

It's great to have this. Should there be an issue at the bokeh repo to gave an API to query anything you currently have hardcoded?

@philippjfr

This comment has been minimized.

Member

philippjfr commented Feb 28, 2017

Should there be an issue at the bokeh repo to gave an API to query anything you currently have hardcoded?

I don't think there's much bokeh can do as long as the layout solver is run client side and once that has run it's too late because the solver may have failed completely.

@jbednar

This comment has been minimized.

Member

jbednar commented Feb 28, 2017

So are these hints, then, not hardcoding? I.e., will the client then figure out the actual values?

@philippjfr

This comment has been minimized.

Member

philippjfr commented Feb 28, 2017

So are these hints, then, not hardcoding? I.e., will the current then figure out the actual values?

No, it will respect those values but has to draw everything inside those bounds, if things get squeezed too tight it blows up and doesn't render.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Mar 6, 2017

@jlstevens Ready to merge.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Mar 6, 2017

Looks good! Merging.

@jlstevens jlstevens merged commit 45afc6e into master Mar 6, 2017

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.09%) to 78.316%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr deleted the bokeh_grid_axes branch Apr 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment