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

Merged
merged 9 commits into from Mar 6, 2017

Conversation

Projects
None yet
3 participants
@philippjfr
Copy link
Contributor

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 force-pushed the bokeh_grid_axes branch 2 times, most recently from 59c79de to 6e891bb Feb 25, 2017

@philippjfr philippjfr removed the WIP label Feb 25, 2017

@philippjfr philippjfr force-pushed the bokeh_grid_axes branch 3 times, most recently from cf8be8d to 970f557 Feb 25, 2017

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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

Contributor

Looks like 50 is a magic number...

This comment has been minimized.

@philippjfr

philippjfr Feb 27, 2017

Author Contributor

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

Contributor

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

Author Contributor

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

Contributor

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

Author Contributor

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

Contributor

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

Contributor

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

This comment has been minimized.

@philippjfr

philippjfr Feb 27, 2017

Author Contributor

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

Contributor

Sounds very fragile.

This comment has been minimized.

@philippjfr

philippjfr Feb 27, 2017

Author Contributor

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

Author Contributor

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

Contributor

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

Author Contributor

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

Author Contributor

(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

Contributor

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

Author Contributor

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

@philippjfr philippjfr force-pushed the bokeh_grid_axes branch 2 times, most recently from 99cb913 to cbd8063 Feb 27, 2017

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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 philippjfr force-pushed the bokeh_grid_axes branch from 2b7fd52 to 3fdda7d Mar 6, 2017

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Mar 6, 2017

@jlstevens Ready to merge.

@jlstevens

This comment has been minimized.

Copy link
Contributor

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
You can’t perform that action at this time.