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

DynamicMap with streams force callback on first lookup after refresh #858

Merged
merged 11 commits into from Sep 12, 2016

Conversation

Projects
None yet
2 participants
@philippjfr
Member

philippjfr commented Sep 12, 2016

Currently the plotting code will request a particular key from a single DynamicMap multiple times during plotting which should be avoided due to potentially random state.

@@ -672,7 +673,8 @@ def __getitem__(self, key):
# Cache lookup
try:
if util.dimensionless_contents(self.streams, self.kdims):
if (util.dimensionless_contents(self.streams, self.kdims) and
not self._stream_cache_lookup):
raise KeyError('Using dimensionless streams disables DynamicMap cache')

This comment has been minimized.

@jlstevens

jlstevens Sep 12, 2016

Member

From our discussion, we think it ought to be something like:

if (util.dimensionless_contents(self.streams, self.kdims)
                  raise KeyError('Using dimensionless streams disables DynamicMap cache')
elif self._disable_cache_lookup:
                  raise KeyError('Cache lookup disabled')

That way the variable isn't about streams and you can disable cache lookup on dynamic maps not matter how they are (or aren't) using streams.

This comment has been minimized.

@philippjfr

philippjfr Sep 12, 2016

Member

I think we decided in the end that it is about streams.

@@ -787,7 +787,7 @@ def stream_parameters(streams, no_duplicates=True, exclude=['name']):
If no_duplicates is enabled, a KeyError will be raised if there are
parameter name clashes across the streams.
"""
param_groups = [s.params().keys() for s in streams]
param_groups = [s.contents.keys() for s in streams]
names = [name for group in param_groups for name in group]

This comment has been minimized.

@jlstevens

jlstevens Sep 12, 2016

Member

Thanks for the fix!

class enable_streams_cache(object):
"""

This comment has been minimized.

@jlstevens

jlstevens Sep 12, 2016

Member

Given the discussion about, I would call it disable_cache_lookup with constructor:

def __init__(self, obj):

I don't think it needs an enable or disable flag as enable is the default behaviour. Unless you can think of situations where you want to be able to disable the context manager with a boolean? If you can think of such a situation (or just think it makes it more flexible) then

def __init__(self, obj, disable=True):

Would be reasonable. The issue with using either enable or disable as an argument name is that it isn't clear if you are enabling/disabling the cache lookup of enabling/disabling the context manager that is enabling/disabling the cache lookup!

For this reason, maybe something like this would be clearer:

def __init__(self, obj, allow_cache_lookup=False):

Just some things to think about...

This comment has been minimized.

@philippjfr

philippjfr Sep 12, 2016

Member

I don't think it needs an enable or disable flag as enable is the default behaviour. Unless you can think of situations where you want to be able to disable the context manager with a boolean? If you can think of such a situation (or just think it makes it more flexible) then

The issue is that making context manager conditional is awkward, which is why I moved the boolean onto the object. I agree with renaming the argument and making it optional though.

@force.setter
def force(self, value):
self.traverse(lambda x: setattr(x, '_force', value))

This comment has been minimized.

@jlstevens

jlstevens Sep 12, 2016

Member

From what I see below, I think having self.force as a plain attribute without self._force would be simpler (i.e no properties). The reason is that in the context_manager you just need to accessself.force. Then you set self.force to True and False in one place in this one file. As a result, I'm not sure it is worth introducing properties and an underscore attribute when you can just inline two fairly short traversal expressions.

This comment has been minimized.

@philippjfr

philippjfr Sep 12, 2016

Member

We decided on replacing this with a traverse_setter utility.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Sep 12, 2016

All looks good now! Happy to merge once the tests pass.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Sep 12, 2016

Tests are passing. Merging.

@jlstevens jlstevens merged commit 42098c4 into master Sep 12, 2016

3 of 4 checks passed

continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.4%) to 71.243%
Details
s3-reference-data-cache Test data is cached.
Details

@jlstevens jlstevens removed the in progress label Sep 12, 2016

@philippjfr philippjfr deleted the dynamic_use_cache branch Sep 27, 2016

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