-
-
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
Optimize display of large DynamicMap parameter space #2646
Conversation
@jlstevens Another good reason to get a 1.10.3 release out. |
holoviews/core/util.py
Outdated
@@ -1327,7 +1328,9 @@ def drop_streams(streams, kdims, keys): | |||
stream_params = stream_parameters(streams) | |||
inds, dims = zip(*[(ind, kdim) for ind, kdim in enumerate(kdims) | |||
if kdim not in stream_params]) | |||
return dims, [tuple(wrap_tuple(key)[ind] for ind in inds) for key in keys] | |||
get = itemgetter(*inds) |
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.
Don't often see a use for itemgetter
and I'm trying to think whether you really need it here. I don't immediately have a better suggestion so it is probably ok...
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.
itemgetter is about 100x faster than the previous tuple comprehension.
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.
That seems to be a good reason. Could you put in a comment to that effect?
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.
Sure, that said itemgetter is used frequently throughout utils, so I'm not sure this one deserves a special note.
I've made one comment and I'm not sure whether you are done with it, but at a glance this PR seems fine. |
Ready to merge. |
Looks 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 described in #2645, a DynamicMap with a large parameter space can be very slow to initialize, this is because we have started to process its keys more consistently but did not take into account the fact that the DynamicMap parameter spaces can be much larger. This now avoids a number of computations that are not necessary and optimized some others:
This DynamicMap defines a parameter space with 2 million keys, previously this would take 52 seconds to display, with this PR this drops to 0.5 seconds.