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

Disable caching of renderer parameters on OutputMagic #1285

Merged
merged 12 commits into from Apr 13, 2017

Conversation

Projects
None yet
2 participants
@philippjfr
Member

philippjfr commented Apr 12, 2017

Additionally ensures that the order of backends supplied to the notebook_extension is respected. Fixes #947.

@philippjfr philippjfr added the notebook label Apr 12, 2017

@@ -84,7 +84,7 @@ class Renderer(Exporter):
The full, lowercase name of the rendering backend or third
part plotting package used e.g 'matplotlib' or 'cairo'.""")
dpi=param.Integer(None, allow_None=True, doc="""
dpi=param.Integer(72, doc="""

This comment has been minimized.

@jlstevens

jlstevens Apr 13, 2017

Member

Do we want to be setting a specific dpi in the Renderer baseclass? That would suggest all backends will want to use this dpi...

This comment has been minimized.

@philippjfr

philippjfr Apr 13, 2017

Member

Good point, I'll move it to MPLRenderer.

for backend, imp in imports:
try:
__import__('holoviews.plotting.%s' % imp)
if selected_backend is None:
selected_backend = backend

This comment has been minimized.

@jlstevens

jlstevens Apr 13, 2017

Member

Good to see it is now obeying the specified order, with the first thing listed activated.

for r in [r for r in resources if r != 'holoviews']:
Store.renderers[r].load_nb(inline=p.inline)
if resources[-1] != 'holoviews':
get_ipython().magic(u"output backend=%r" % resources[-1]) # noqa (get_ipython))

This comment has been minimized.

@jlstevens

jlstevens Apr 13, 2017

Member

Good to get rid of this..it was hackish.

('css' , None)])
# Defines the options the OutputMagic remembers the remainder
# is simply state on the backend specific Renderer

This comment has been minimized.

@jlstevens

jlstevens Apr 13, 2017

Member

'OutputMagic remembers the remainder is simply state'? Either punctuation or words are missing...

This comment has been minimized.

@jlstevens

jlstevens Apr 13, 2017

Member

Maybe..

# Defines the options the OutputMagic remembers. All other options 
# are held by the backend specific Renderer.

Which is what I think you meant...

cls._set_render_options(cls.defaults)
cls.set_backend(backend)
cls.options = dict({k: cls.defaults[k] for k in cls.remembered})
cls._set_render_options({}, backend)

This comment has been minimized.

@jlstevens

jlstevens Apr 13, 2017

Member

Why do you need to call _set_render_options if there are no options? I suppose there is state being initialize there?

This comment has been minimized.

@philippjfr

philippjfr Apr 13, 2017

Member

I'll have a look at what I can do.

This comment has been minimized.

@jlstevens

jlstevens Apr 13, 2017

Member

It was more of a question than a request. But if you see a way to improve it further, great!

prev_backend = Store.current_backend
renderer = Store.renderers[Store.current_backend]
renderer = Store.renderers[prev_backend]

This comment has been minimized.

@jlstevens

jlstevens Apr 13, 2017

Member

I would move these three line:

prev_backend = Store.current_backend
renderer = Store.renderers[prev_backend]
prev_backend += ':%s' % renderer.mode

Just after the block starting with the # Get new backend comment and before # Update allowed formats.

Seeing a method starting with 'prev_backend = Store.current_backend' just seems weird but once you've seen the code about getting the new backend it makes more sense. As far as I can tell, these variables don't seem to be referenced before that point.

This comment has been minimized.

@philippjfr

philippjfr Apr 13, 2017

Member

Hmm, so you just want to switch Get new backend and Get previous backend blocks around?

This comment has been minimized.

@jlstevens

jlstevens Apr 13, 2017

Member

I think it is a bit more readable that way. If there is an issue swapping them around, don't bother...

# Get new backend
backend = items.get('backend', Store.current_backend)
split = backend.split(':')
core_backend, mode = split if len(split)==2 else (split[0], 'default')

This comment has been minimized.

@jlstevens

jlstevens Apr 13, 2017

Member

It would be nice if we had a word that meant 'backend plus mode' so that 'backend' is the thing without the mode.

This comment has been minimized.

@jlstevens

jlstevens Apr 13, 2017

Member

Then we could get rid of 'core_backend' as a name (which I don't like). Not sure what terminology would make more sense. Maybe backend+mode is a backend_spec?

This comment has been minimized.

@philippjfr
self._set_render_options(render_params, backend)
if backend.split(':')[0] != prev_backend:
self.set_backend(prev_backend)
self._set_render_options(prev_params, prev_backend+':'+prev_mode)

This comment has been minimized.

@jlstevens

jlstevens Apr 13, 2017

Member

I'm tempted to say something about context managers. Maybe for another day and another PR...

prev_mode = prev_renderer.mode
prev_params = {k: v for k, v in prev_renderer.get_param_values()
if k in self.render_params}
prev_restore = dict(OutputMagic.options)

This comment has been minimized.

@jlstevens

jlstevens Apr 13, 2017

Member

Maybe prev is confusing as it suggests the backend is changing (which it might not). Maybe initial_backend? Implement this if you like the suggestion, it isn't something worth holding up the PR for.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 13, 2017

I've made a few comments. It mostly looks fine and I'm happy to merge once you've seen my suggestions and decided whether to use them or not.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 13, 2017

Looks good and the tests are passing. Merging.

@jlstevens jlstevens merged commit f9c23b5 into master Apr 13, 2017

3 of 4 checks passed

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

@philippjfr philippjfr deleted the disable_output_magic_cache branch Apr 19, 2017

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