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

Dynamic Callable API #951

Merged
merged 10 commits into from
Oct 31, 2016
Merged

Dynamic Callable API #951

merged 10 commits into from
Oct 31, 2016

Conversation

philippjfr
Copy link
Member

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

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

Choose a reason for hiding this comment

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

to extract

@jlstevens
Copy link
Contributor

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure.

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Using no_duplicates=False would be clearer here...

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure.

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

Choose a reason for hiding this comment

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

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



def get_streams(dmap):
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Would get_nested_streams or nested_streams be a better name?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure.

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

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not quite but it could definitely be refactored more nicely.

Copy link
Contributor

@jlstevens jlstevens Oct 31, 2016

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@philippjfr philippjfr Oct 31, 2016

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

No because Dynamic performs some wrapping around the operation.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Made the changes you suggested.

@jlstevens
Copy link
Contributor

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
@philippjfr philippjfr deleted the dynamic_callable branch January 7, 2017 15: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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants