Skip to content
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

Defined hv.extension and hv.renderer utilities #1517

Merged
merged 10 commits into from Jun 5, 2017

Conversation

Projects
None yet
2 participants
@jlstevens
Copy link
Contributor

jlstevens commented Jun 5, 2017

Defines hv.renderer which helps load rendering backends outside of a notebook context.

@jlstevens jlstevens force-pushed the renderer_util branch from 734ae1b to fa2151c Jun 5, 2017

@@ -361,7 +361,7 @@ def update_options(cls, options, items):
@classmethod
def initialize(cls, backend_list):
cls.backend_list = backend_list
backend = cls.options.get('backend', Store.current_backend)
backend = Store.current_backend

This comment has been minimized.

@jlstevens

jlstevens Jun 5, 2017

Author Contributor

@philippjfr I don't quite know why I had to make this change. I'll try to summarize:

  • In the magic tests, %unload_ext is called which meant that initialize was called multiple times because that unsets _loaded on the notebook extension. If the options dictionary had a value, this meant state was bleeding across tests and breaking.
  • I can't tell what changed: I believe initialize would have been called multiple times in the tests before this PR too.

That said, why isn't this an appropriate fix? When you initialize, it should reset the state even if initialize was called before. Why use a value from cls.options when the purpose here is to reset it to the defaults (line below?).

This comment has been minimized.

@philippjfr

philippjfr Jun 5, 2017

Contributor

I can't be sure tbh, this code was always fairly brittle and I had to reorganize it a few times so methods may just have been badly named. I'd strongly suggest you make a notebook and then switch back and forth between backends with output a few times, rerun the notebook_extension, set fig formats then switch backends and so on. I don't think tests are currently a sufficient indication that things are still working.

This comment has been minimized.

@jlstevens

jlstevens Jun 5, 2017

Author Contributor

Sure. I can do that to check. That said, those switches shouldn't be going through initialize each time anyway.

This comment has been minimized.

@jlstevens

jlstevens Jun 5, 2017

Author Contributor

I'd strongly suggest you make a notebook and then switch back and forth between backends with output a few times, rerun the notebook_extension, set fig formats then switch backends and so on.

I've just done that (included rerunning notebook_extension) and it seems to be working correctly.

This comment has been minimized.

@philippjfr

philippjfr Jun 5, 2017

Contributor

Okay happy to merge once test pass.

@@ -254,6 +191,24 @@ def _get_resources(self, args, params):
resources = ['holoviews'] + resources
return resources

@classmethod
def load_hvjs(cls, logo=False, JS=True, message='HoloViewsJS successfully loaded.'):

This comment has been minimized.

@jlstevens

jlstevens Jun 5, 2017

Author Contributor

@philippjfr Is this ok? Anything else using this (i.e users)?

This comment has been minimized.

@philippjfr

philippjfr Jun 5, 2017

Contributor

What am I meant to check here? Looks like you just moved it slightly.

This comment has been minimized.

@jlstevens

jlstevens Jun 5, 2017

Author Contributor

Just wondering if anything else might have been using it. Why wasn't it a classmethod to begin with?

@jlstevens jlstevens changed the title Defined hv.renderer utility Defined hv.extension utility Jun 5, 2017

@jlstevens jlstevens force-pushed the renderer_util branch from cd501f0 to 673da72 Jun 5, 2017

@jlstevens jlstevens changed the title Defined hv.extension utility Defined hv.extension and hv.renderer utilities Jun 5, 2017

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Jun 5, 2017

@philippjfr I think this PR is ready for review once the tests are green.

@philippjfr

This comment has been minimized.

Copy link
Contributor

philippjfr commented Jun 5, 2017

Looks good to me.

@philippjfr philippjfr merged commit 6945ce7 into master Jun 5, 2017

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 79.257%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr deleted the renderer_util branch Jun 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.