-
-
Notifications
You must be signed in to change notification settings - Fork 402
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reimplemented this because assigning this way is inefficient for an internal operation.
6f03df2
to
4b00354
Compare
f7b655a
to
6e3ee9f
Compare
Would like to get deep-select and |
4111a84
to
fa5d797
Compare
@jlstevens This still needs a lot of unit tests for the |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this because len(self.dimensions())
includes cdims
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because it can include the deep dimensions as well which should be processed below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.""") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to explain the new data_slice
argument in the docstring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 Dynamic Maybe 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stray line that can be removed? (sliced=None
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, thanks.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forget...is there a good reason why this can't be a lambda? (replacing dynamic_slice
?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No probably could be.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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. |
As outlined in #422 this PR implements some further methods on DynamicMap, ensuring that the
relabel
andredim
methods can be used correctly on a DynamicMap. Dynamic methods like this are easily implemented using theDynamic
utility, which currently lives inhv.util
. Since this forces me to use inline imports I'm now considering movingDynamic
into core.