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 plot option to transpose layouts #1100

Merged
merged 3 commits into from Feb 6, 2017

Conversation

Projects
None yet
3 participants
@philippjfr
Member

philippjfr commented Feb 5, 2017

As requested in #909 this adds a plot option to transpose layouts. It was trivial to implement and provides useful functionality.

@jbednar

This comment has been minimized.

Member

jbednar commented Feb 5, 2017

Thanks so much!

@@ -1030,6 +1030,9 @@ class GenericLayoutPlot(GenericCompositePlot):
displays the elements in a cartesian grid in scanline order.
"""
transpose = param.Boolean(default=False, doc="""
Whether to transpose the layout when plotting""")

This comment has been minimized.

@jlstevens

jlstevens Feb 5, 2017

Member

Would be good to say the normal behavior is scanline order (left to right, top to bottom) and that transposing makes it work top to bottom and left to right.

This comment has been minimized.

@philippjfr

philippjfr Feb 5, 2017

Member

Sounds good, I'll make sure to add some tests too.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Feb 5, 2017

Other than a suggestion for improving the docstring, this looks good and I'm happy to merge.

@philippjfr philippjfr added this to the v1.7.0 milestone Feb 6, 2017

@jbednar

This comment has been minimized.

Member

jbednar commented Feb 6, 2017

I can't tell from the code -- will this make the subfigure labels transpose as well? Presumably we'd want them to, because if the ordering in the layout has any meaning, the layout's order will now be top to bottom instead of left to right.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Feb 6, 2017

I can't tell from the code -- will this make the subfigure labels transpose as well? Presumably we'd want them to, because if the ordering in the layout has any meaning, the layout's order will now be top to bottom instead of left to right.

True, currently not the case, it simply labels as normal now.

@jbednar

This comment has been minimized.

Member

jbednar commented Feb 6, 2017

I can't tell from the code -- will this make the subfigure labels transpose as well? Presumably we'd want them to, because if the ordering in the layout has any meaning, the layout's order will now be top to bottom instead of left to right.

True, currently not the case, it simply labels as normal now.

It's a start, then, but changing the label order seems dangerous to put off until later, because if we ever do implement that, a previously labeled transposed figure will no longer match its caption.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Feb 6, 2017

Now handles the subfigure labels too. Ready to merge when tests pass.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Feb 6, 2017

All tests are green. Merging.

@jlstevens jlstevens merged commit 32758c4 into master Feb 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.1%) to 78.131%
Details
s3-reference-data-cache Test data is cached.
Details
@jbednar

This comment has been minimized.

Member

jbednar commented Feb 6, 2017

Sorry I didn't get a chance to test this earlier, but I think this is a problem: the cols() method now seems to determine the number of rows, which is very unintuitive:

image

Shouldn't cols() still determine the number of columns? Being able to determine the number of rows is nice, but would seem to require having a method other than cols().

The code in that example is from https://github.com/bokeh/colorcet/blob/master/docs/index.ipynb , where the definition of colormaps is replaced with the above.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Feb 6, 2017

Shouldn't cols() still determine the number of columns? Being able to determine the number of rows is nice, but would seem to require having a method other than cols().

It could do, but it's not really a transpose in that case, and the implementation would be a lot uglier.

@jbednar

This comment has been minimized.

Member

jbednar commented Feb 6, 2017

Drat. If you look at that example notebook as updated with the new colormaps function above, you'll see that the transpose option doesn't really work, then -- the idea was to have a fixed number of columns, but to start to fill them top to bottom, not left to right. As it is now, sometimes there are two columns, sometimes 50, which doesn't work from a layout perspective because the width of a web page is fixed:

image

I.e., a fixed number of columns is about the only thing that makes sense on a webpage that scrolls vertically; cramming in all the plots left to right doesn't seem like it would normally be useful in practice.

@jbednar

This comment has been minimized.

Member

jbednar commented Feb 6, 2017

It can be faked by having setting the cols() to a fraction of the length of the layout, but that's very fragile.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Feb 6, 2017

It can be faked by having setting the cols() to a fraction of the length of the layout, but that's very fragile.

True, it depends on the layout not changing length. I'll investigate what the alternative looks like.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Feb 6, 2017

I'm wondering if the (Generic)LayoutPlot could use a bit of modulo arithmetic on the number of elements to convert the cols value appropriately (if transposed).

@jbednar

This comment has been minimized.

Member

jbednar commented Feb 6, 2017

This definition of cols seems to work fine:

def colormaps(named_cms,cols=3,array=xs):
    images = [hv.Image(array, group=name)(style=dict(cmap=cm)) 
              for name,cm in named_cms]
    return hv.Layout(images)(plot=dict(transpose=True)).display('all').cols(int(np.ceil(len(images)*1.0/cols)))

So is it possible to stick that when the columns value is used, so that it uses that version instead of the cols number directly, if it's transposed?

@jlstevens

This comment has been minimized.

Member

jlstevens commented Feb 6, 2017

So is it possible to stick that when the columns value is used, so that it uses that version instead of the cols number directly, if it's transposed?

Right. That is what I was suggesting...and now you've shown a concrete implementation, I can't see why the plotting class can't use similar logic.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Feb 6, 2017

I can't see why the plotting class can't use similar logic.

It probably can easily enough, I'll look into it.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Feb 6, 2017

One annoyance is that we have the current Layout logic encoded in Layout.shape and NdLayout.shape. I'd vote for either deprecating those or actually allow transposing at the object level, e.g. by extending the cols method.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Feb 6, 2017

I don't think Layout.shape is public API so I am happy to see it changed. Changing cols is a different matter though but I am open to any backwards-compatible approach.

@philippjfr philippjfr deleted the layout_transpose branch Feb 10, 2017

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