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

Merged
merged 3 commits into from
Feb 6, 2017
Merged

Add plot option to transpose layouts #1100

merged 3 commits into from
Feb 6, 2017

Conversation

philippjfr
Copy link
Member

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

@jbednar
Copy link
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""")
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@jlstevens
Copy link
Contributor

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
Copy link
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
Copy link
Member Author

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
Copy link
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
Copy link
Member Author

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

@jlstevens
Copy link
Contributor

All tests are green. Merging.

@jlstevens jlstevens merged commit 32758c4 into master Feb 6, 2017
@jbednar
Copy link
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
Copy link
Member Author

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
Copy link
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
Copy link
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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
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
Copy link
Contributor

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
Copy link
Member Author

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

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

@philippjfr
Copy link
Member Author

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
Copy link
Contributor

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 February 10, 2017 01:01
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants