Implement dynamic relabel, redim and select methods #1029
Conversation
| 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) |
There was a problem hiding this comment.
Reimplemented this because assigning this way is inefficient for an internal operation.
philippjfr
force-pushed
the
dynamic_methods
branch
from
6f03df2
to
4b00354
Compare
philippjfr
changed the title
philippjfr
force-pushed
the
dynamic_methods
branch
from
f7b655a
to
6e3ee9f
Compare
|
Would like to get deep-select and |
philippjfr
force-pushed
the
dynamic_methods
branch
from
4111a84
to
fa5d797
Compare
|
@jlstevens This still needs a lot of unit tests for the |
philippjfr
changed the title
| else self.ndims) | ||
| ndims = self.ndims | ||
| if any(d in self.vdims for d in kwargs): | ||
| ndims = len(self.kdims+self.vdims) |
There was a problem hiding this comment.
Is this because len(self.dimensions()) includes cdims?
There was a problem hiding this comment.
No, because it can include the deep dimensions as well which should be processed below.
| @@ -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.""") | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
.data on a DynamicMap is not the callback but the cache. Will update the docstring to reflect that.
There was a problem hiding this comment.
Ah yes, thanks! I suppose you might want to copy the cache in some cases (e.g for relabel).
There was a problem hiding this comment.
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 | |||
There was a problem hiding this comment.
Would be good to explain the new data_slice argument in the docstring.
There was a problem hiding this comment.
Agreed, docstrings still need updating. I'd love it if there was some way to inherit method docstrings easily, when you override a method.
|
Instead of the inline imports: from ..util import DynamicMaybe it is time we add |
I think |
| 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): |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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)There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Stray line that can be removed? (sliced=None)
| from ..util import Dynamic | ||
| def dynamic_slice(obj): | ||
| return obj[data_slice] | ||
| return Dynamic(product, operation=dynamic_slice, shared_data=True) |
There was a problem hiding this comment.
I forget...is there a good reason why this can't be a lambda? (replacing dynamic_slice?)
Ok, go for it! Edit: You can see I accidentally closed the PR after posting this! |
| else: | ||
| from ..util import Dynamic | ||
| def dynamic_slice(obj): | ||
| return obj[data_slice] |
There was a problem hiding this comment.
Can also be replaced with a lambda...
|
Looks good! I've finished making my current set of suggestions for this PR. I do think adding |
I was wrong, the circular imports between dynamic.py and operation.py would be pretty bad. |
|
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. |
|
All looking good! Merging. |
As outlined in #422 this PR implements some further methods on DynamicMap, ensuring that the
relabelandredimmethods can be used correctly on a DynamicMap. Dynamic methods like this are easily implemented using theDynamicutility, which currently lives inhv.util. Since this forces me to use inline imports I'm now considering movingDynamicinto core.