Disable caching of renderer parameters on OutputMagic #1285

philippjfr · 2017-04-12T18:35:10Z

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

jlstevens · 2017-04-13T01:19:11Z

holoviews/plotting/renderer.py

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


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

Good point, I'll move it to MPLRenderer.

jlstevens · 2017-04-13T01:21:42Z

holoviews/ipython/__init__.py

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


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

jlstevens · 2017-04-13T01:22:29Z

holoviews/ipython/__init__.py

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


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

jlstevens · 2017-04-13T01:23:56Z

holoviews/ipython/magics.py

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


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

Maybe..

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

Which is what I think you meant...

jlstevens · 2017-04-13T01:32:19Z

holoviews/ipython/magics.py

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


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

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

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

jlstevens · 2017-04-13T16:49:08Z

holoviews/ipython/magics.py

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


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.

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

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

jlstevens · 2017-04-13T16:49:57Z

holoviews/ipython/magics.py

+        # 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')


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

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?

jlstevens · 2017-04-13T16:52:42Z

holoviews/ipython/magics.py

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


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

jlstevens · 2017-04-13T16:54:45Z

holoviews/ipython/magics.py

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


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 · 2017-04-13T16:58:07Z

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 · 2017-04-13T18:06:43Z

Looks good and the tests are passing. Merging.

github-actions · 2024-10-25T22:22:01Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

philippjfr added the tag: notebook label Apr 12, 2017

philippjfr force-pushed the disable_output_magic_cache branch 4 times, most recently from a21165b to 7a12f53 Compare April 12, 2017 20:56

jlstevens reviewed Apr 13, 2017

View reviewed changes

jlstevens mentioned this pull request Apr 13, 2017

Re-enable option system keyword validation #1277

Merged

philippjfr force-pushed the disable_output_magic_cache branch 2 times, most recently from 02fcd48 to 771e7fd Compare April 13, 2017 15:02

philippjfr added 9 commits April 13, 2017 16:26

Disabled caching of Renderer parameters on output magic

6022442

Ensure the first backend supplied to notebook_extension is selected

f1fcfee

Fixes for OutputMagic allowed handling

60195c6

Fixed OutputMagic unit tests

cf574dc

Defined default dpi on Renderer

152f7d8

Ensure renderer params are restored by OutputMagic

3fc3a98

Moved default dpi definition to MPLRenderer

2482289

Small fix to comment in OutputMagic

5e7df55

Completely revised OutputMagic logic

aa3d88b

philippjfr force-pushed the disable_output_magic_cache branch from 771e7fd to aa3d88b Compare April 13, 2017 15:26

jlstevens reviewed Apr 13, 2017

View reviewed changes

philippjfr added 2 commits April 13, 2017 18:13

Ensure notebook_extension displays baseclass Renderer assets

9f4a82e

Small fix for notebook_extension backend loading

bfdd81a

Improved naming of variables in OutputMagic

e4031b4

jlstevens merged commit f9c23b5 into master Apr 13, 2017

philippjfr deleted the disable_output_magic_cache branch April 19, 2017 21:38

github-actions bot locked as resolved and limited conversation to collaborators Oct 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable caching of renderer parameters on OutputMagic #1285

Disable caching of renderer parameters on OutputMagic #1285

philippjfr commented Apr 12, 2017 •

edited

Loading

jlstevens Apr 13, 2017

philippjfr Apr 13, 2017

jlstevens Apr 13, 2017

jlstevens Apr 13, 2017

jlstevens Apr 13, 2017

jlstevens Apr 13, 2017

jlstevens Apr 13, 2017

philippjfr Apr 13, 2017

jlstevens Apr 13, 2017

jlstevens Apr 13, 2017

philippjfr Apr 13, 2017

jlstevens Apr 13, 2017 •

edited

Loading

jlstevens Apr 13, 2017

jlstevens Apr 13, 2017

philippjfr Apr 13, 2017

jlstevens Apr 13, 2017

jlstevens Apr 13, 2017

jlstevens commented Apr 13, 2017

jlstevens commented Apr 13, 2017

github-actions bot commented Oct 25, 2024

Disable caching of renderer parameters on OutputMagic #1285

Disable caching of renderer parameters on OutputMagic #1285

Conversation

philippjfr commented Apr 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlstevens Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlstevens commented Apr 13, 2017

jlstevens commented Apr 13, 2017

github-actions bot commented Oct 25, 2024

philippjfr commented Apr 12, 2017 •

edited

Loading

jlstevens Apr 13, 2017 •

edited

Loading