-
-
Notifications
You must be signed in to change notification settings - Fork 404
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
Conversation
if self.p.dynamic: | ||
def dynamic_operation(key): | ||
return self(element[key], **params) | ||
processed = self._make_dynamic(element, dynamic_operation) |
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.
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.
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
A single cast = param.ObjectSelector(default=None, objects=[None, 'static', 'dynamic']) Then instead of I think another way would be to specify the container class eg |
Don't particularly like |
Adds DynamicOperation to allow applying any function dynamically to a HoloMap or DynamicMap and used it to apply operations dynamically in ElementOperation
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 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]) |
623927d
to
5fbaccc
Compare
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) |
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 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.
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.
Sounds good, happy to do this and shouldn't take long.
from .operation import DynamicOperation | ||
def dynamic_mul(element): | ||
return element * other | ||
return DynamicOperation(self, dynamic_mul) |
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.
This inner function is so small, why not use a lambda?
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.
Prefer to use a proper function because at least it is named. Don't really care though.
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. |
I'm happy with the latest changes and I am ready to merge. I wish I could think of a name better than 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. |
Don't have any immediate suggestion. As you say, we can always rename it later as it's mostly an internal detail for now. |
I understand it is now ready to merge. |
This build dependency should not be required but conda build fails without it. See PR #588 on conda-forge/staged-recipes for more information.
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 the title says, this adds a
dynamic
parameter to theElementOperation
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.