Fixed no memoization bug if stream provides no kwargs #3070
Conversation
|
Ready to review and merge. |
philippjfr
force-pushed
the
no_kwarg_memoize
branch
from
5583e96 to
6d5787c
Compare
philippjfr
force-pushed
the
no_kwarg_memoize
branch
from
6d5787c to
f9b63a7
Compare
holoviews/core/spaces.py
Outdated
| @@ -542,7 +542,7 @@ def __call__(self, *args, **kwargs): | |||
| # Nothing to do for callbacks that accept no arguments | |||
| kwarg_hash = kwargs.pop('memoization_hash', ()) | |||
There was a problem hiding this comment.
How come the hash is passed in via the kwargs? Does that mean that if a user tries to use a stream with the parameter 'memoization_hash', it will not work as expected?
There was a problem hiding this comment.
Ah, I suppose the Callable gets it, but at least the callback does not. Even so, it means Callable has kwargs that won't be passed to the callback (if supplied by the user).
There was a problem hiding this comment.
How come the hash is passed in via the kwargs? Does that mean that if a user tries to use a stream with the parameter 'memoization_hash', it will not work as expected?
It's the only way we have of letting streams define hash keys to the Callable that are not simply the same as the contents and has been that way for a while. It is true that if a user stream has a parameter called memoization_hash then that won't work properly. Seems like a highly unlikely clash but if you don't think it's safe I'd be happy to add an underscore or something.
Even so, it means Callable has kwargs that won't be passed to the callback (if supplied by the user).
Don't quite follow this, when or why would a user call the Callable?
There was a problem hiding this comment.
Don't quite follow this, when or why would a user call the Callable
I don't expect this to be a common case, but ideally the Callable is a relatively transparent wrapper around the callback. This isn't a blocker for this PR though...
|
Looks good with the updated kwarg name. Merging. |
# Conflicts: # holoviews/tests/teststreams.py
|
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. |
Our recent work on supporting
param.dependsallows defining DynamicMaps which do not have any args or kwargs, since the parameters are available on theselfin the parameterized method callback. However there is a small bug inCallablewhich skips memoization if both args and kwargs are empty rather than checking thekwarg_hashwhich is what is actually memoized on.