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

Dynamic Callable API #951

Merged
merged 10 commits into from Oct 31, 2016

Conversation

Projects
None yet
3 participants
@philippjfr
Member

philippjfr commented Oct 24, 2016

As described in #895, we needed a way to access streams on DynamicMaps wrapping other DynamicMaps or overlays containing one or more DynamicMaps. This PR implements a Callable object, which is used to wrap the DynamicMap callbacks generated by all internal operations. This makes it possible to access streams and stream sources on nested DynamicMaps and hook them up appropriately in the plotting code.

Currently the Callable object is very straightforward, exposing a callable_function and objects parameter, which correspond to the callback and the list of objects the callback wraps around, e.g. in an overlaying operation between a DynamicMap and a Points Elements, the callable_function would be a function which overlays the two objects and the objects a list containing both objects.

Using two helper functions get_streams and get_sources we can then look up all the streams and sources that are associated with a particular plot and more specifically a specific layer in an Overlay.

The PR is still lacking docstrings and tests but it already works.

The best example to confirm it is working is this example of two separate selection callbacks attached to two separate data sources:

%%opts Points [tools=['box_select', 'lasso_select', 'tap']]
points = hv.Points(np.random.multivariate_normal((0, 0), [[1, 0.1], [0.1, 1]], (1000,)))
points2 = hv.Points(np.random.multivariate_normal((0, 0), [[1, 0.1], [0.1, 1]], (1000,)))

sel1 = Selection1D()
sel2 = Selection1D()
hv.DynamicMap(lambda index: points, kdims=[], streams=[sel1]) *\
hv.DynamicMap(lambda index: hv.HLine(points['y'][index].mean() if index else -10), kdims=[], streams=[sel1]) *\
hv.DynamicMap(lambda index: points2, kdims=[], streams=[sel2]) *\
hv.DynamicMap(lambda index: hv.HLine(points2['y'][index].mean() if index else -10), kdims=[], streams=[sel2])

dual_selection

@jlstevens

This comment has been minimized.

Member

jlstevens commented Oct 30, 2016

I've skimmed the PR and now I'm looking at it in detail. Overall it looks fine but I need to think about the implications for the user quite carefully.

My first thought was...could we offer a decorator that users can easily use to wrap any callable? A decorator would look nice and clean and would be very easy for the user to add at any point. In fact, couldn't this transformation be applied dynamically? For instance, the wrapped callback could be an underscore attribute in DynamicMap as soon as it is declared so that the user doesn't even have to worry or know about any of this?

Perhaps you have already done all this this and I'll find out once I look a bit more closely!

that the operation and the objects that are part of the operation
are made available. This allows traversing a DynamicMap that wraps
multiple operations and is primarily used to extracting any
streams attached to the object.

This comment has been minimized.

@jbednar

jbednar Oct 30, 2016

Member

to extract

@jlstevens

This comment has been minimized.

Member

jlstevens commented Oct 30, 2016

@philippjfr I would be interested to see the same example you posted above as DynamicMap * DynamicMap. I think that would give me a better insight into how it works as well as a taste of what users might want to do in some situations.

def get_streams(dmap):
"""
Get streams from DynamicMap with Callable callback.
"""

This comment has been minimized.

@jlstevens

jlstevens Oct 31, 2016

Member

Might say something like 'Get all (potentially nested) streams ...'

This comment has been minimized.

@philippjfr
return Dynamic(other, operation=dynamic_mul)
callback = Callable(callable_function=dynamic_mul,
inputs=[self, other])
return other.clone(shared_data=False, callback=callback,

This comment has been minimized.

@jlstevens

jlstevens Oct 31, 2016

Member

Wondering whether it should be self.clone, or other.clone or maybe a new DynamicMap declaration entirely. I see this is in the condition where other is a DynamicMapbut is this definitely right in terms of kdims? I need to think about it more...

This comment has been minimized.

@jlstevens

jlstevens Oct 31, 2016

Member

Can't this stuff relying on other being DynamicMap be moved to _dynamic_mul? There is already this condition in __mul__:

if isinstance(self, DynamicMap) or isinstance(other, DynamicMap):
    return self._dynamic_mul(dimensions, other, super_keys)

If all the logic regarding dynamic could move to _dynamic_mul, that would be cleaner...

This comment has been minimized.

@philippjfr

philippjfr Oct 31, 2016

Member

Wondering whether it should be self.clone, or other.clone or maybe a new DynamicMap declaration entirely. I see this is in the condition where other is a DynamicMapbut is this definitely right in terms of kdims?

Yes, this is the condition where self is a single Element or Overlay.

If all the logic regarding dynamic could move to _dynamic_mul, that would be cleaner...

This is the __mul__ implementation on Overlayable, it doesn't have _dynamic_mul, because I'd like to avoid inline imports.

@@ -689,7 +726,8 @@ def __getitem__(self, key):
# Cache lookup
try:
dimensionless = util.dimensionless_contents(self.streams, self.kdims)
dimensionless = util.dimensionless_contents(get_streams(self),
self.kdims, False)
if (dimensionless and not self._dimensionless_cache):

This comment has been minimized.

@jlstevens

jlstevens Oct 31, 2016

Member

Using no_duplicates=False would be clearer here...

This comment has been minimized.

@philippjfr
@@ -204,7 +204,7 @@ def _validate(self, obj, fmt):
if (((len(plot) == 1 and not plot.dynamic)
or (len(plot) > 1 and self.holomap is None) or
(plot.dynamic and len(plot.keys[0]) == 0)) or
not unbound_dimensions(plot.streams, plot.dimensions)):
not unbound_dimensions(plot.streams, plot.dimensions, False)):

This comment has been minimized.

@jlstevens

jlstevens Oct 31, 2016

Member

Again, no_duplicates=False would be clearer here...

def get_streams(dmap):
"""

This comment has been minimized.

@jlstevens

jlstevens Oct 31, 2016

Member

Would get_nested_streams or nested_streams be a better name?

This comment has been minimized.

@philippjfr
Traverses Callable graph to resolve sources on
DynamicMap objects, returning a list of sources
indexed by the Overlay layer.
"""

This comment has been minimized.

@jlstevens

jlstevens Oct 31, 2016

Member

Can't this be simplified to:

if isinstance(obj, DynamicMap) and isinstance(obj.callback, Callable):
    return [(index, obj)]

leaving the rest of the function to handle DynamicMaps with Callable callbacks?

This comment has been minimized.

@philippjfr

philippjfr Oct 31, 2016

Member

Not quite but it could definitely be refactored more nicely.

This comment has been minimized.

@jlstevens

jlstevens Oct 31, 2016

Member

Ah...given it is:

 +   if isinstance(obj, DynamicMap):
 +        if isinstance(obj.callback, Callable):
                  ....
 +        else:
 +            return [(index, obj)]
 +    else:
 +        return [(index, obj)]

I think I meant:

if not isinstance(obj, DynamicMap) or not isinstance(obj.callback, Callable):
    return [(index, obj)]
@@ -108,7 +108,11 @@ def __init__(self, plot, renderer=None, **params):
super(NdWidget, self).__init__(**params)
self.id = plot.comm.target if plot.comm else uuid.uuid4().hex
self.plot = plot
self.dimensions, self.keys = drop_streams(plot.streams,

This comment has been minimized.

@jlstevens

jlstevens Oct 31, 2016

Member

I assume this bit is simply a bug fix and otherwise unrelated?

This comment has been minimized.

@philippjfr

philippjfr Oct 31, 2016

Member

Before this wasn't an issue, so not unrelated.

This comment has been minimized.

@jlstevens

jlstevens Oct 31, 2016

Member

Ok...seems to be a result of plot.streams potentially having many more instances due to the nesting than before...

This comment has been minimized.

@philippjfr

philippjfr Oct 31, 2016

Member

Right in these cases it just needs to know if a kdim has a corresponding stream or not, there is no actual clash at the DynamicMap level because each level of wrapping, i.e. all operations you apply, resolve their own streams.

self.p.kwargs.update(kwargs)
_, el = util.get_dynamic_item(map_obj, map_obj.kdims, key)
return self._process(el, key)
if isinstance(self.p.operation, ElementOperation):

This comment has been minimized.

@jlstevens

jlstevens Oct 31, 2016

Member

Couldn't self.p.operation be folded into callable_function so you only need Callable and not OperationCallable?

This comment has been minimized.

@philippjfr

philippjfr Oct 31, 2016

Member

No because Dynamic performs some wrapping around the operation.

This comment has been minimized.

@jlstevens

jlstevens Oct 31, 2016

Member

I'm a little confused. _dynamic_operation seems to be called once here and I don't see the operation parameter then being inspected/used on the callback variable anywhere in Dynamic. I am yet to spot where the operation parameter of OperationCallable is used...so I'm assuming I've missed it.

This comment has been minimized.

@philippjfr

philippjfr Oct 31, 2016

Member

It is never used, but is useful information and will in future be used to look up the outputs.

This comment has been minimized.

@philippjfr

philippjfr Oct 31, 2016

Member

Ah sorry, I'm confused different bit of code.

This comment has been minimized.

@philippjfr

philippjfr Oct 31, 2016

Member
if isinstance(self.p.operation, ElementOperation):
            kwargs = {k: v for k, v in self.p.kwargs.items()
                      if k in self.p.operation.params()}
            return self.p.operation.process_element(element, key, **kwargs)
if type(stream) in registry and streams}
sources = [(self.zorder, self.hmap.last)]
cb_classes = set()
for _, source in sources:

This comment has been minimized.

@jlstevens

jlstevens Oct 31, 2016

Member

So the key difference seems to be that you can now have multiple sources whereas before there was only one. And the sources are now retrieved recursively via get_sources which works via the Callable instances...

@philippjfr

This comment has been minimized.

Member

philippjfr commented Oct 31, 2016

Made the changes you suggested.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Oct 31, 2016

Ok, I feel I mostly understand it now... the most magical bit (i.e the bit I couldn't figure out!) seems to be in get_sources but everything else makes sense.

I'll merge now on the understanding that we now have a plan to improve the user API side of things. This PR shows that this approach will allow nested DynamicMaps to work properly with streams.

@jlstevens jlstevens merged commit d74a10f into master Oct 31, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@philippjfr philippjfr deleted the dynamic_callable branch Jan 7, 2017

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