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
Merged

Widgets ignore dimension streams #860

merged 5 commits into from Sep 12, 2016

Conversation

@philippjfr
Copy link
Member

@philippjfr 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.

@@ -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 Member

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

@jlstevens
Copy link
Contributor

@jlstevens 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
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
@philippjfr
s3-reference-data-cache Test data is cached.
Details
@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
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants