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

Implement dynamic relabel, redim and select methods #1029

Merged
merged 14 commits into from Jan 7, 2017

Conversation

Projects
None yet
2 participants
@philippjfr
Member

philippjfr commented Dec 27, 2016

As outlined in #422 this PR implements some further methods on DynamicMap, ensuring that the relabel and redim methods can be used correctly on a DynamicMap. Dynamic methods like this are easily implemented using the Dynamic utility, which currently lives in hv.util. Since this forces me to use inline imports I'm now considering moving Dynamic into core.

if (depth > 0) and getattr(obj, '_deep_indexable', False):
for k, v in obj.items():
obj[k] = v.relabel(group=group, label=label, depth=depth-1)

This comment has been minimized.

@philippjfr

philippjfr Dec 27, 2016

Member

Reimplemented this because assigning this way is inefficient for an internal operation.

@philippjfr philippjfr changed the title from Implement dynamic relabel, map and redim methods to Implement dynamic relabel and redim methods Dec 28, 2016

@philippjfr philippjfr added this to the v1.7.0 milestone Dec 28, 2016

@philippjfr

This comment has been minimized.

Member

philippjfr commented Dec 29, 2016

Would like to get deep-select and __getitem__ into this PR before merge.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jan 7, 2017

@jlstevens This still needs a lot of unit tests for the select and __getitem__ methods, but the actual implementation is now ready for review.

@philippjfr philippjfr changed the title from Implement dynamic relabel and redim methods to Implement dynamic relabel, redim and select methods Jan 7, 2017

else self.ndims)
ndims = self.ndims
if any(d in self.vdims for d in kwargs):
ndims = len(self.kdims+self.vdims)

This comment has been minimized.

@jlstevens

jlstevens Jan 7, 2017

Member

Is this because len(self.dimensions()) includes cdims?

This comment has been minimized.

@philippjfr

philippjfr Jan 7, 2017

Member

No, because it can include the deep dimensions as well which should be processed below.

This comment has been minimized.

@jlstevens

jlstevens Jan 7, 2017

Member

Ok. Makes sense...

@@ -28,14 +28,17 @@ class Dynamic(param.ParameterizedFunction):
kwargs = param.Dict(default={}, doc="""
Keyword arguments passed to the function.""")
shared_data = param.Boolean(default=False, doc="""
Whether the cloned DynamicMap will share the same data.""")

This comment has been minimized.

@jlstevens

jlstevens Jan 7, 2017

Member

As .data for a DynamicMap is the callback, it would be good to explain what sharing/not sharing means here. I'm not quite sure what it means to be honest so it might be something we want to document in other places too.

This comment has been minimized.

@philippjfr

philippjfr Jan 7, 2017

Member

.data on a DynamicMap is not the callback but the cache. Will update the docstring to reflect that.

This comment has been minimized.

@jlstevens

jlstevens Jan 7, 2017

Member

Ah yes, thanks! I suppose you might want to copy the cache in some cases (e.g for relabel).

This comment has been minimized.

@philippjfr

philippjfr Jan 7, 2017

Member

Right, it's probably mostly going to be used internally.

@@ -655,7 +654,7 @@ def reset(self):
return self
def _cross_product(self, tuple_key, cache):
def _cross_product(self, tuple_key, cache, data_slice):
"""
Returns a new DynamicMap if the key (tuple form) expresses a
cross product, otherwise returns None. The cache argument is a

This comment has been minimized.

@jlstevens

jlstevens Jan 7, 2017

Member

Would be good to explain the new data_slice argument in the docstring.

This comment has been minimized.

@philippjfr

philippjfr Jan 7, 2017

Member

Agreed, docstrings still need updating. I'd love it if there was some way to inherit method docstrings easily, when you override a method.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jan 7, 2017

Instead of the inline imports:

   from ..util import Dynamic

Maybe it is time we add core.dynamic which includes DynamicMap and Dynamic? Or would this just result in different import issues?

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jan 7, 2017

Maybe it is time we add core.dynamic which includes DynamicMap and Dynamic? Or would this just result in different import issues?

I think HoloMap.__mul__ would have to inline import it, but I think that may be still be preferable, so I'd vote for it.

self._cache(tuple_key, val)
return val
def select(self, selection_specs=None, **kwargs):
selection = super(DynamicMap, self).select(selection_specs, **kwargs)
def dynamic_select(obj):

This comment has been minimized.

@jlstevens

jlstevens Jan 7, 2017

Member

Would dynamic_select have any use on its own in util? It seems to offer a select that works with both dynamic and non dynamic objects...(or at least as an operation that can be used to select on dynamic objects).

elif key == ():
map_slice, data_slice = (), ()
else:
map_slice, data_slice = self._split_index(key)

This comment has been minimized.

@jlstevens

jlstevens Jan 7, 2017

Member

Any chance _split_index could support the empty tuple directly? What happens with an empty tuple currently? It would be nice if this bit could be:

if key is Ellipsis:
    return self
 map_slice, data_slice = self._split_index(key)

This comment has been minimized.

@philippjfr

philippjfr Jan 7, 2017

Member

NdMapping.__getitem__ does this with empty tuples:

elif indexslice == () and not self.kdims:
     return self.data[()]

Won't work here unfortunately, but I guess I could still handle it in _split_index, NdMapping would just never use that path.

raise Exception("Slices must be used exclusively or not at all")
if slices:
return self._slice_bounded(tuple_key)
sliced = None

This comment has been minimized.

@jlstevens

jlstevens Jan 7, 2017

Member

Stray line that can be removed? (sliced=None)

This comment has been minimized.

@philippjfr

philippjfr Jan 7, 2017

Member

Yup, thanks.

from ..util import Dynamic
def dynamic_slice(obj):
return obj[data_slice]
return Dynamic(product, operation=dynamic_slice, shared_data=True)

This comment has been minimized.

@jlstevens

jlstevens Jan 7, 2017

Member

I forget...is there a good reason why this can't be a lambda? (replacing dynamic_slice?)

This comment has been minimized.

@philippjfr

philippjfr Jan 7, 2017

Member

No probably could be.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jan 7, 2017

I think HoloMap.mul would have to inline import it, but I think that may be still be preferable, so I'd vote for it.

Ok, go for it!

Edit: You can see I accidentally closed the PR after posting this!

@jlstevens jlstevens closed this Jan 7, 2017

@jlstevens jlstevens reopened this Jan 7, 2017

else:
from ..util import Dynamic
def dynamic_slice(obj):
return obj[data_slice]

This comment has been minimized.

@jlstevens

jlstevens Jan 7, 2017

Member

Can also be replaced with a lambda...

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jan 7, 2017

Looks good!

I've finished making my current set of suggestions for this PR. I do think adding core.dynamic makes sense to do now as core.spaces is getting quite long...

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jan 7, 2017

Ok, go for it!

I was wrong, the circular imports between dynamic.py and operation.py would be pretty bad.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jan 7, 2017

Shame... any other suggestions for breaking up this file and splitting the dynamic stuff out?

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jan 7, 2017

Shame... any other suggestions for breaking up this file and splitting the dynamic stuff out?

Would have to think about it. Might open an issue.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jan 7, 2017

All looking good! Merging.

@jlstevens jlstevens merged commit 43774fc into master Jan 7, 2017

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 76.821%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr deleted the dynamic_methods branch Jan 27, 2017

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