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

Added support for dynamic operations and overlaying #588

Merged
merged 9 commits into from Apr 11, 2016

Conversation

Projects
None yet
2 participants
@philippjfr
Member

philippjfr commented Apr 1, 2016

As the title says, this adds a dynamic parameter to the ElementOperation class, which let's the user apply lazy analyses to a HoloMap (or GridSpace of HoloMaps), which will only be evaluated when the user requests a specific key. This makes it trivial to apply complex analyses to large datasets without having to apply the analysis everywhere at once and avoids having to manually wrap the analysis in a dynamic callback.

if self.p.dynamic:
def dynamic_operation(key):
return self(element[key], **params)
processed = self._make_dynamic(element, dynamic_operation)

This comment has been minimized.

@jlstevens

jlstevens Apr 4, 2016

Member

Minor suggestion: I might consider using the signature _make_dynamic(hmap, key, params) and moving the nested function into that method (or using a lambda). This works as the function used is always generated by calling self.

This is a very mild preference and if you prefer to leave it as it is, I don't mind either.

else:
raise ValueError("Cannot process type %r" % type(element).__name__)
return processed
def _make_dynamic(self, hmap, dynamic_fn):

This comment has been minimized.

@jlstevens

jlstevens Apr 4, 2016

Member

Actually, how about making this a general utility to turn HoloMaps into DynamicMaps? That seems like it could be useful and would justify keeping the existing signature (overriding my suggestion above).

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 4, 2016

Looks good and I can see how it would be useful. I've made two small comments.

There is only one other thing I think is necessary for this to be complete (though this PR can be merged as a step towards this goal). If the input is a DynamicMap I would expect it to return a DynamicMap by default. The question then is what the dynamic parameter means. The behaviour I would expect:

  • HoloMap -> HoloMap (default)
  • HoloMap -> DynamicMap (if explicitly requested)
  • DynamicMap -> DynamicMap (default)
  • DynamicMap -> HoloMap (if explicitly requested)

A single dynamic parameter doesn't capture both those defaults. I would rather avoid having to always supply dynamic=True when working with DynamicMap. How about some other parameter, something like:

cast = param.ObjectSelector(default=None, objects=[None, 'static', 'dynamic'])

Then instead of operation(hmap, dynamic=True) it would be operation(hmap, cast='dynamic')?

I think another way would be to specify the container class eg operation(hmap, container=hv.HoloMap) which might be nicer in some ways but could potentially be more verbose. At least hv.HoloMap and hv.DynamicMap would tab complete!

@philippjfr

This comment has been minimized.

Member

philippjfr commented Apr 9, 2016

Don't particularly like cast as a name because it's not casting anything. container is also confusing to me as you could be passing in a GridSpace of HoloMaps and it would turn the HoloMaps into DynamicMaps but it's not obvious what container refers to in that case. I'd prefer sticking with dynamic and set it to either None or 'default'by default, then it defaults to what you pass in but you can explicitly override it.

Support for applying operations to DynamicMaps
Adds DynamicOperation to allow applying any function dynamically
to a HoloMap or DynamicMap and used it to apply operations dynamically
in ElementOperation
@philippjfr

This comment has been minimized.

Member

philippjfr commented Apr 9, 2016

I've now factored out a DynamicOperation, which takes a HoloMap/DynamicMap and a function as input and returns a DynamicMap, which will apply that function lazily to each Element as it is loaded. The ElementOperation simply makes use of this to apply itself to its input. I've gone with setting dynamic to 'default' and allowing the user to explicitly force it to True/False.

Here's a nice little example:

hv.operation.contours(hv.DynamicMap(lambda x: hv.Image(np.random.rand(10,10))),
                      levels=[0, 0.25, 0.5, 0.75, 1])

image

@philippjfr philippjfr changed the title from Added dynamic option to ElementOperations to Added support for dynamic operations and overlaying Apr 9, 2016

@philippjfr

This comment has been minimized.

Member

philippjfr commented Apr 10, 2016

As the title says, I've also folded in dynamic overlaying which turned out not to be a major problem having implemented the DynamicOperation. I think have the basic operators work is pretty important and we can focus on the remaining methods on DynamicMap at another point. This is ready to merge once you've reviewed it.

"""
def __call__(self, map_obj, function, **kwargs):
callback = self._dynamic_operation(map_obj, function, **kwargs)

This comment has been minimized.

@jlstevens

jlstevens Apr 11, 2016

Member

I would prefer it if __call__ has the same signature as Operation and if instead of passing in function as an argument you use the _process method. This would make DynamicOperation more consistent with Operation and you could always create a very generic DynamicOperation subclass that takes in a function from the outside to achieve the same functionality.

As a naming suggestion, I might consider naming this subclass DynamicFunction to make it clear that it takes in a function. Once this is done, the docstring of this class should read 'Dynamically applies an operation to the elements of a HoloMap...' and the docstring of DynamicFunction will be the docstring above.

This comment has been minimized.

@philippjfr

philippjfr Apr 11, 2016

Member

Sounds good, happy to do this and shouldn't take long.

specific frame is requested, specified as a Boolean. If set to
'default' the mode will be determined based on the input type,
i.e. if the data is a DynamicMap it will stay dynamic.""")

This comment has been minimized.

@jlstevens

jlstevens Apr 11, 2016

Member

I'm happy with this approach.

@@ -97,6 +98,56 @@ def _dimension_keys(self):
for k in self.keys()]
def _dynamic_mul(self, dimensions, other, keys):
"""

This comment has been minimized.

@jlstevens

jlstevens Apr 11, 2016

Member

Glad to see this functionality but sadly it makes __mul__ into even more of a monster. I've always felt that __mul__ should be implemented once as a separate utility/operation one day. If you agree, I can file an issue to suggest this.

This comment has been minimized.

@philippjfr

philippjfr Apr 11, 2016

Member

I do agree, a separate utility makes sense to me, so please do open an issue.

This comment has been minimized.

@jlstevens

jlstevens Apr 11, 2016

Member

Done. See issue #600.

key = self.hmap.keys()[-1]
else:
frame = None
key, frame = util.get_dynamic_item(self.hmap, self.dimensions, key)

This comment has been minimized.

@jlstevens

jlstevens Apr 11, 2016

Member

Nice to see this as a utility!

Looks up an item in a DynamicMap given a list of dimensions
and a corresponding key. The dimensions must be a subset
of the map_obj key dimensions.
"""

This comment has been minimized.

@jlstevens

jlstevens Apr 11, 2016

Member

An example would be nice in the docstring here. Doesn't have to be an actual doctest if that is too tricky to get working...

This comment has been minimized.

@philippjfr

philippjfr Apr 11, 2016

Member

Will do, can add a unit test too.

@@ -78,3 +79,55 @@ def test_sampled_bounded_resample(self):
dmap=DynamicMap(fn, sampled=True)
self.assertEqual(dmap[{0, 1, 2}].keys(), [0, 1, 2])
class DynamicTestOperation(ComparisonTestCase):

This comment has been minimized.

@jlstevens

jlstevens Apr 11, 2016

Member

Great! Thanks for adding these tests!

from .operation import DynamicOperation
def dynamic_mul(element):
return element * other
return DynamicOperation(self, dynamic_mul)

This comment has been minimized.

@jlstevens

jlstevens Apr 11, 2016

Member

This inner function is so small, why not use a lambda?

This comment has been minimized.

@philippjfr

philippjfr Apr 11, 2016

Member

Prefer to use a proper function because at least it is named. Don't really care though.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 11, 2016

Looks good and I'm very happy with the new functionality here. I've made some comments, none of which should take long to implement (or respond to). Once addressed, I'm happy to merge.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 11, 2016

I'm happy with the latest changes and I am ready to merge. I wish I could think of a name better than DynamicFunction though!

If you have any suggestions, it would be good to get that in now. If now, we can always rename it in a later PR after the merge.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Apr 11, 2016

Don't have any immediate suggestion. As you say, we can always rename it later as it's mostly an internal detail for now.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 11, 2016

I understand it is now ready to merge.

@jlstevens jlstevens merged commit 5fb7e95 into master Apr 11, 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.1%) to 69.689%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr removed the in progress label Apr 11, 2016

@philippjfr philippjfr deleted the dynamic_ops branch Apr 20, 2016

jlstevens added a commit that referenced this pull request Jun 29, 2016

Added numpy to build section of meta.yaml
This build dependency should not be required but conda build fails
without it. See PR #588 on conda-forge/staged-recipes for more
information.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment