-
-
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
Fixed scrubber bug and add support for discrete DynamicMap scrubber #1832
Conversation
@jlstevens Ready to review. The basics of it is that a DynamicMap which defines a discrete sampling can be treated just the same as a HoloMap, which also means you scrub through the parameter space. The unique_dimkeys method computes the unique keys by taking the cartesian product of the DynamicMap dimension values. Additionally it fixes a small bug in |
holoviews/core/dimension.py
Outdated
@@ -639,7 +639,11 @@ def map(self, map_fn, specs=None, clone=True): | |||
for k, v in self.items(): | |||
new_val = v.map(map_fn, specs, clone) | |||
if new_val is not None: | |||
deep_mapped[k] = new_val | |||
# Ensure key validation doesn't cause errors |
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 don't quite get why there would be key errors: deep_mapped
is a clone of self
and k
comes from self.items()
so why would the key ever be rejected?
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.
Just had to double check to figure out what's going on, apparently this can occur when working with multiple DynamicMaps in a Layout. I've not been able to track down the actual bug but it seems something else is bypassing the validation so you end up with a cache that contains values it shouldn't. I'll try to track down the original bug that leads to the invalid key being inserted in the first place.
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.
Found it, DynamicMap is inserting stuff into the cache with checking first.
values = [d.values for d in all_dims] | ||
if obj.traverse(lambda x: x, ['DynamicMap']) and values and all(values): | ||
unique_keys += list(zip(*cartesian_product(values))) | ||
|
||
with item_check(False): | ||
sorted_keys = NdMapping({key: None for key in unique_keys}, | ||
kdims=all_dims).data.keys() |
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.
Why do we do this? Is this more useful than taking the set of keys then sorting them?
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.
There used to be at least, not sure its the case anymore though. Might leave a note to simplify this, but it's
used in a few places.
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.
Actually I'll open an issue because this is related to sorting issues.
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.
Opened the issue here: #1835
@@ -301,15 +301,16 @@ def get_widget(self_or_cls, plot, widget_type, **kwargs): | |||
if not isinstance(plot, Plot): | |||
plot = self_or_cls.get_plot(plot) | |||
dynamic = plot.dynamic | |||
# Whether dimensions define discrete space | |||
discrete = all(d.values for d in plot.dimensions) |
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 seems to be the basic condition that determines when scrubber can be used (with DynamicMap
).
Looks good - I just asked a few questions in the comments. Not sure why the |
8f3fcdd
to
8be0d5b
Compare
Thanks for the comments, I've reverted the change to |
Looks good. Merging. |
Addresses #1567 fixing a bug in the way scrubber handled looping. Additionally also adds support for using the scrubber when a DynamicMap defines
values
for all its dimensions, which effectively makes it discrete and therefore the same as a Holomap.