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

Widgets ignore dimension streams #860

Merged
merged 5 commits into from Sep 12, 2016

Conversation

Projects
None yet
2 participants
@philippjfr
Copy link
Contributor

philippjfr commented Sep 12, 2016

Widgets now ignore dimensions which are defined through a stream ensuring that only widgets for non-stream dimensions are displayed.

Philipp Rudiger Philipp Rudiger
@@ -824,6 +832,16 @@ def wrap_tuple_streams(unwrapped, kdims, streams):
return tuple(substituted)


def drop_streams(streams, keys, kdims):
"""
Drop any dimensionsed streams from the keys and kdims.

This comment has been minimized.

@jlstevens

jlstevens Sep 12, 2016

Contributor

Typo in 'dimensioned' and maybe add a line saying what it returns.

This comment has been minimized.

@jlstevens

jlstevens Sep 12, 2016

Contributor

I still think it is a little odd to put the keys (typically a very long list) before the kdims (much shorter). The order that feels (slightly) more natural to me for the signature is (streams, kdims, keys). As there can be a long list of streams, the next best is probably (kdims, streams, keys).

Just a minor point that you may ignore if you wish!



def wrap_tuple_streams(unwrapped, kdims, streams):
"""
Fills in tuple keys with dimensioned stream values as appropriate.
"""
param_groups = [(s.params().keys(), s) for s in streams]
param_groups = [(s.contents.keys(), s) for s in streams]
pairs = [(name,s) for (group, s) in param_groups for name in group]

This comment has been minimized.

@jlstevens

jlstevens Sep 12, 2016

Contributor

Thanks! Made the same mistake twice!

self.plot[key]
self.plot.push()
return '' if self.renderer.mode == 'nbagg' else 'Complete'

This comment has been minimized.

@jlstevens

jlstevens Sep 12, 2016

Contributor

Nice to see this stuff go away!

self.update(0)
key = self.current_key if self.current_key else self.keys[0]
stream_key = util.wrap_tuple_streams(key, self.dimensions, self.streams)
self.update(stream_key)
if self.comm is not None:

This comment has been minimized.

@jlstevens

jlstevens Sep 12, 2016

Contributor

It is a relief to see how simple this bit turned out to be.

@@ -920,6 +920,9 @@ def __init__(self, layout, keys=None, dimensions=None, **params):
**params)
if top_level:
self.comm = self.init_comm(layout)
self.streams = [s for streams in layout.traverse(lambda x: x.streams,
[DynamicMap])

This comment has been minimized.

@jlstevens

jlstevens Sep 12, 2016

Contributor

The wrapping is a bit weird. I would put in layout.traverse(lambda x: x.streams, [DynamicMap]) onto the second line and for s in streams onto the third line.



def streamless_dimensions(streams, kdims):
"""

This comment has been minimized.

@jlstevens

jlstevens Sep 12, 2016

Contributor

I feel we need a better word for streamless_dimensions. These used to be simply 'key dimensions' before streams came along!

In other words, I would like a way to define these things in terms of what they are instead of what they are not. I can't say I have any great suggestions right now 'widget_dimensions', 'requested_dimensions', 'unbound_dimensions'? Got any ideas?

self.keys = plot.keys

dims, keys = drop_streams(plot.streams, plot.keys, plot.dimensions)
self.dimensions, self.keys = dims, keys

This comment has been minimized.

@jlstevens

jlstevens Sep 12, 2016

Contributor

As the variables dims and keys don't seem to be used in this method for anything other than setting self.dimensions and self.keys, I would consider using:

self.dimensions, self.keys = drop_streams(plot.streams, 
                                          plot.keys, 
                                          plot.dimensions)
return self._plot_figure(key)
self.plot.update(key)
self.plot.push()
return 'Complete'

This comment has been minimized.

@jlstevens

jlstevens Sep 12, 2016

Contributor

'Update Complete' maybe? Is 'Complete' by itself informative enough? I'm assuming this might be something that appears in the console. If it is a message/part of a protocol, it is probably not worth changing.

This comment has been minimized.

@philippjfr

philippjfr Sep 12, 2016

Author Contributor

Currently part of the protocol, which will change as soon as we use comms for everything.

Philipp Rudiger Philipp Rudiger
@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Sep 12, 2016

Tests have passed so I will merge it now. I'm excited to try it out!

@jlstevens jlstevens merged commit becf225 into master Sep 12, 2016

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.009%) to 71.259%
Details
s3-reference-data-cache Test data is cached.
Details

@jlstevens jlstevens removed the in progress label Sep 12, 2016

@philippjfr philippjfr deleted the stream_dimension_widgets branch Oct 14, 2016

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.