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

Merged
merged 10 commits into from Aug 31, 2017

Conversation

Projects
None yet
2 participants
@philippjfr
Member

philippjfr commented Aug 30, 2017

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.

@philippjfr philippjfr added the bug label Aug 30, 2017

@philippjfr

This comment has been minimized.

Member

philippjfr commented Aug 31, 2017

@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 .map and various bugs in the scrubber JS code left over from when we had support for generator mode.

@@ -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

This comment has been minimized.

@jlstevens

jlstevens Aug 31, 2017

Member

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?

This comment has been minimized.

@philippjfr

philippjfr Aug 31, 2017

Member

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.

This comment has been minimized.

@philippjfr

philippjfr Aug 31, 2017

Member

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()

This comment has been minimized.

@jlstevens

jlstevens Aug 31, 2017

Member

Why do we do this? Is this more useful than taking the set of keys then sorting them?

This comment has been minimized.

@philippjfr

philippjfr Aug 31, 2017

Member

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.

This comment has been minimized.

@philippjfr

philippjfr Aug 31, 2017

Member

Actually I'll open an issue because this is related to sorting issues.

This comment has been minimized.

@philippjfr

philippjfr Aug 31, 2017

Member

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)

This comment has been minimized.

@jlstevens

jlstevens Aug 31, 2017

Member

This seems to be the basic condition that determines when scrubber can be used (with DynamicMap).

@jlstevens

This comment has been minimized.

Member

jlstevens commented Aug 31, 2017

Looks good - I just asked a few questions in the comments. Not sure why the s3-reference-data-cache isn't green yet but the other tests have passed and I am happy to merge.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Aug 31, 2017

Thanks for the comments, I've reverted the change to .map and ensured that DynamicMap correctly validates its keys and that it doesn't warn about KeyError because that's the accepted signal to declare a key value as invalid (and display an empty frame).

@jlstevens

This comment has been minimized.

Member

jlstevens commented Aug 31, 2017

Looks good. Merging.

@jlstevens jlstevens merged commit 656e42f into master Aug 31, 2017

3 of 4 checks passed

s3-reference-data-cache Tests still building.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls First build on master at 79.658%
Details

@philippjfr philippjfr deleted the scrubber_fix branch Sep 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment