Various fixes for bokeh static_source optimization #1413
Conversation
|
Looks good. I can't really see how to test this right now so I'll merge once the tests pass. |
|
So this approach does not work since it just disables the optimization entirely. The issue is twofold a) when a custom style is applied the element id changes, b) something in the plotting pipeline applies a dynamic method to the data, which again ends up changing the element id. Therefore the plotting code never sees the same element id twice and the optimization does nothing. Will have to think about this. |
|
Problem b) seems like the real issue to me as I don't think disabling the optimization for a) means it will always be disabled. |
philippjfr
force-pushed
the
static_source_opt_fix
branch
from
28350a3 to
bdc69a7
Compare
holoviews/core/dimension.py
Outdated
| @@ -488,6 +489,7 @@ def __init__(self, data, id=None, **params): | |||
| """ | |||
| self.data = data | |||
| self.id = id | |||
| self._plot_id = plot_id or obj_id(self) | |||
There was a problem hiding this comment.
Just use __builtins__.id to avoid the name shadowing issue.
| @@ -488,6 +489,7 @@ def __init__(self, data, id=None, **params): | |||
| """ | |||
| self.data = data | |||
| self.id = id | |||
There was a problem hiding this comment.
Do we ever actually set the id via the constructor?
There was a problem hiding this comment.
Ah, probably for clone. Nevermind my question!
There was a problem hiding this comment.
Yes, clone does all the time.
holoviews/plotting/bokeh/element.py
Outdated
| else: | ||
| current_id = id(element.data) | ||
| current_id = element._plot_id |
There was a problem hiding this comment.
Is it no longer taking into account the id of the data? It seems to do that in tabular...
| @@ -147,7 +147,7 @@ def sync_sources(self): | |||
| from the same object. | |||
| """ | |||
| get_sources = lambda x: (id(x.current_frame.data), x) | |||
| filter_fn = lambda x: (x.shared_datasource and x.current_frame and | |||
| filter_fn = lambda x: (x.shared_datasource and x.current_frame is not None and | |||
There was a problem hiding this comment.
Just to confirm: is this a a fix unrelated to the optimization itself?
There was a problem hiding this comment.
Seems so, not sure why I pushed it here, but it is an important fix.
holoviews/plotting/bokeh/tabular.py
Outdated
| previous_id = self.handles.get('previous_id', None) | ||
| current_id = id(self.current_frame.data) if self.current_frame else None | ||
| current_id = None if self.current_frame is None else id(element)+id(element.data) |
There was a problem hiding this comment.
Summing ids is not a safe test for uniqueness as many pairs of ids could sum to the same value (even if it is unlikely!). Note that collisions might not even be that rare as the id doesn't return a hash - it is more like to a memory address.
To check the identity of both the element and the data I would use the tuple (id(element), id(element.data)) and not the sum.
|
Looks good with the latest fixes. One last import issue to fix and then I'll merge once the tests go green. That said, are there any meaningful unit tests we should set up to test the optimization? |
philippjfr
force-pushed
the
static_source_opt_fix
branch
from
a80c3cb to
43b3c38
Compare
philippjfr
force-pushed
the
static_source_opt_fix
branch
from
43b3c38 to
a203695
Compare
|
Thanks for adding the unit tests! 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. |
In #1396 it was suggested that the
static_sourceoptimization in bokeh should only kick in if both the element and its data are static.