-
-
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
Add memoization to dynamic Callable class #1063
Conversation
Wow! And the total amount of code even went down. Fabulous! |
@jlstevens Requested review, the code is fairly straightforward and I've added at least one test to ensure the memoization behaves correctly. There are already a number of tests to check memoization works during plotting, stopping the callback from being called repeatedly. |
to a DynamicMap. | ||
to a DynamicMap. Additionally a Callable will memoize the last | ||
returned value based on the arguments to the function and the | ||
state of all streams on its inputs, avoiding calling the function |
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 think either 'avoiding calls to the function' or 'to avoid calling the function ...' would be read better.
to a DynamicMap. Additionally a Callable will memoize the last | ||
returned value based on the arguments to the function and the | ||
state of all streams on its inputs, avoiding calling the function | ||
unncessarily. | ||
""" |
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.
Typo.
|
||
|
||
class dimensionless_cache(object): | ||
""" |
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.
Glad to get rid of this!
@@ -500,6 +517,8 @@ class DynamicMap(HoloMap): | |||
""") | |||
|
|||
def __init__(self, callback, initial_items=None, **params): | |||
if not isinstance(callback, Callable) and not isinstance(callback, types.GeneratorType): | |||
callback = Callable(callable_function=callback) |
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 means all callbacks will now be wrapped in Callable
. This is a change from the previous behavior, and probably a good one (more consistent, at least with __mull__
) though we should now document this. It would also be good to have an example of where a user might want to supply Callable
themselves....
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.
For instance, we might want an example of the user declaring a Callable
with their own set of inputs
.
def __call__(self, *args, **kwargs): | ||
return self.callable_function(*args, **kwargs) | ||
inputs = [inp for inp in self.inputs if isinstance(inp, DynamicMap)] |
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.
Very minor point - I would call the loop variable i
given that inp
is a bit weird and you probably don't want to clash with the input
built-in. Alternatively, this is one instance of a pure filter
so you could consider using that (it would be shorter).
return self.callable_function(*args, **kwargs) | ||
inputs = [inp for inp in self.inputs if isinstance(inp, DynamicMap)] | ||
streams = [s for inp in inputs for s in get_nested_streams(inp)] | ||
values = tuple(tuple(sorted(s.contents.items())) for s in streams) |
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 means it is memoizing on the stream parameters in ascending alphanumeric order. I think that is fine (and makes sense!) but it is worth noting that this is one particular policy for how streams parameters are ordered into a tuple key. This is important to stay consistent if we ever need it somewhere else and I am now wondering if it might be worth having a utility to codify the idea....up to you.
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.
It really doesn't matter much, it just needs to be consistently ordered. If we're using it somewhere else it can use the same scheme or another scheme without having any effect here.
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.
True - my point is that we should try to be consistent anyway!
Looks good! I'm very happy this turned out to be far more straightforward than I anticipated. Overall only seven lines were added and the new approach is far more intuitive to me than I think my only worry is that is that (from what I understand) |
That's accurate although each level already caches these values in the DynamicMap cache so I don't think there will be much being cached additionally. It's also worth noting that each level simply wraps the previous level, so it's not holding copies of the data, so the overhead should simply be in the parameterized objects that are created, which should be fairly cheap. |
e40e0fc
to
8a701c8
Compare
Tests are passing so I will merge now. |
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. |
In order to address #1049 the Callable class now memoizes the last returned value from a DynamicMap, based on the args, kwargs and nested stream values.
Needs docstrings and tests. @jordansamuels this seems to address your issue. Small example for testing: