-
-
Notifications
You must be signed in to change notification settings - Fork 401
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
Changes from 6 commits
47135c4
766a40e
d6fbde3
fa2151c
0eab145
cfba05f
673da72
d8dc640
651c2f1
b8986e6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @philippjfr I don't quite know why I had to make this change. I'll try to summarize:
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. I can do that to check. That said, those switches shouldn't be going through There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I've just done that (included rerunning There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay happy to merge once test pass. |
||
if backend in Store.renderers: | ||
cls.options = dict({k: cls.defaults[k] for k in cls.remembered}) | ||
cls.set_backend(backend) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@philippjfr Is this ok? Anything else using this (i.e users)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What am I meant to check here? Looks like you just moved it slightly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering if anything else might have been using it. Why wasn't it a classmethod to begin with?