Disable caching of renderer parameters on OutputMagic #1285
Conversation
philippjfr
force-pushed
the
disable_output_magic_cache
branch
4 times, most recently
from
a21165b
to
7a12f53
Compare
holoviews/plotting/renderer.py
Outdated
| @@ -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=""" | |||
There was a problem hiding this comment.
Do we want to be setting a specific dpi in the Renderer baseclass? That would suggest all backends will want to use this dpi...
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Good to see it is now obeying the specified order, with the first thing listed activated.
holoviews/ipython/__init__.py
Outdated
| 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)) |
There was a problem hiding this comment.
Good to get rid of this..it was hackish.
holoviews/ipython/magics.py
Outdated
| ('css' , None)]) | ||
|
|
||
| # Defines the options the OutputMagic remembers the remainder | ||
| # is simply state on the backend specific Renderer |
There was a problem hiding this comment.
'OutputMagic remembers the remainder is simply state'? Either punctuation or words are missing...
There was a problem hiding this comment.
Maybe..
# Defines the options the OutputMagic remembers. All other options
# are held by the backend specific Renderer.
Which is what I think you meant...
holoviews/ipython/magics.py
Outdated
| 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) |
There was a problem hiding this comment.
Why do you need to call _set_render_options if there are no options? I suppose there is state being initialize there?
There was a problem hiding this comment.
I'll have a look at what I can do.
There was a problem hiding this comment.
It was more of a question than a request. But if you see a way to improve it further, great!
philippjfr
force-pushed
the
disable_output_magic_cache
branch
2 times, most recently
from
02fcd48
to
771e7fd
Compare
philippjfr
force-pushed
the
disable_output_magic_cache
branch
from
771e7fd
to
aa3d88b
Compare
holoviews/ipython/magics.py
Outdated
| prev_backend = Store.current_backend | ||
| renderer = Store.renderers[Store.current_backend] | ||
| renderer = Store.renderers[prev_backend] |
There was a problem hiding this comment.
I would move these three line:
prev_backend = Store.current_backend
renderer = Store.renderers[prev_backend]
prev_backend += ':%s' % renderer.modeJust 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.
There was a problem hiding this comment.
Hmm, so you just want to switch Get new backend and Get previous backend blocks around?
There was a problem hiding this comment.
I think it is a bit more readable that way. If there is an issue swapping them around, don't bother...
holoviews/ipython/magics.py
Outdated
| # 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') |
There was a problem hiding this comment.
It would be nice if we had a word that meant 'backend plus mode' so that 'backend' is the thing without the mode.
There was a problem hiding this comment.
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?
holoviews/ipython/magics.py
Outdated
| 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) |
There was a problem hiding this comment.
I'm tempted to say something about context managers. Maybe for another day and another PR...
holoviews/ipython/magics.py
Outdated
| 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) |
There was a problem hiding this comment.
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.
|
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. |
|
Looks good and the tests are passing. Merging. |
Additionally ensures that the order of backends supplied to the notebook_extension is respected. Fixes #947.