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

Backend-specific option ignored #4962

Closed
douglas-raillard-arm opened this issue Jun 2, 2021 · 14 comments
Closed

Backend-specific option ignored #4962

douglas-raillard-arm opened this issue Jun 2, 2021 · 14 comments
Assignees
Labels
type: bug Something isn't correct or isn't working
Milestone

Comments

@douglas-raillard-arm
Copy link
Contributor

douglas-raillard-arm commented Jun 2, 2021

ALL software version info

holoviews: 1.14.3
matplotlib: 3.4.2
bokeh: 2.3.2

Description of expected behavior and the observed behavior

Disclaimer: I'm a new user and I had to dive straight pretty deep to support my use case, API misuse is not unlikely.

When setting backend-specific options for a non-default backend, the calls to options() seem to override each-other.

Complete, minimal, self-contained example code that reproduces the issue

import holoviews as hv
backend = 'bokeh'
# If the backend that is actually used is the default (i.e. bokeh), the bug disappears
hv.extension('matplotlib', 'bokeh')
print(hv.Store.current_backend)

# The 2nd options() call seems to override the first one
x = hv.Curve([1,2,3]).options(interpolation='steps-pre', backend=backend).options(color='red', backend=backend)
hv.output(x, backend=backend)

# When setting both options at once, the bug disappears
x = hv.Curve([1,2,3]).options(interpolation='steps-pre', color='red', backend=backend)
hv.output(x, backend=backend)

Screenshots or screencasts of the bug in action

image

@jbednar jbednar added the type: bug Something isn't correct or isn't working label Jun 2, 2021
@jbednar
Copy link
Member

jbednar commented Jun 2, 2021

I can reproduce, both with .options and with .opts. That's very surprising!

@jlstevens
Copy link
Contributor

Agreed. Definitely looks like a bug.

@douglas-raillard-arm
Copy link
Contributor Author

It looks like changing the default backend with hv.Store.set_current_backend(backend) prior to constructing the holoviews elements provides a workaround. I wrapped it in a context manager:

                @contextlib.contextmanager
                def set_backend(backend):
                    old_backend = hv.Store.current_backend
                    try:
                        hv.Store.set_current_backend(backend)
                        yield
                    finally:
                        if old_backend:
                            hv.Store.set_current_backend(old_backend)

@jbednar
Copy link
Member

jbednar commented Jun 2, 2021

That's a good suggestion, and a useful bit of code I'd be happy to see in HoloViews (potential PR?). I'd maybe call it just backend so that the usual context manager usage would read as with hv.backend("bokeh"):. But the underlying issue should also be fixed in HoloViews itself.

@jlstevens jlstevens added this to the v2.0 milestone Jun 7, 2021
@douglas-raillard-arm
Copy link
Contributor Author

I'll see what I can do wrt to PR, as there is a bunch of other things I'd like to give a go at fixing. It may take a (long) while though, as I need to seek legal approval from my company first.

douglas-raillard-arm added a commit to douglas-raillard-arm/lisa that referenced this issue Jul 1, 2021
douglas-raillard-arm added a commit to ARM-software/lisa that referenced this issue Jul 1, 2021
douglas-raillard-arm added a commit to douglas-raillard-arm/lisa that referenced this issue Jul 1, 2021
douglas-raillard-arm added a commit to douglas-raillard-arm/lisa that referenced this issue Jul 1, 2021
douglas-raillard-arm added a commit to douglas-raillard-arm/lisa that referenced this issue Jul 1, 2021
douglas-raillard-arm added a commit to douglas-raillard-arm/lisa that referenced this issue Jul 5, 2021
douglas-raillard-arm added a commit to douglas-raillard-arm/lisa that referenced this issue Jul 5, 2021
douglas-raillard-arm added a commit to douglas-raillard-arm/lisa that referenced this issue Jul 6, 2021
douglas-raillard-arm added a commit to douglas-raillard-arm/lisa that referenced this issue Jul 12, 2021
douglas-raillard-arm added a commit to douglas-raillard-arm/lisa that referenced this issue Jul 13, 2021
douglas-raillard-arm added a commit to douglas-raillard-arm/lisa that referenced this issue Jul 13, 2021
douglas-raillard-arm added a commit to douglas-raillard-arm/lisa that referenced this issue Jul 13, 2021
douglas-raillard-arm added a commit to ARM-software/lisa that referenced this issue Jul 13, 2021
douglas-raillard-arm pushed a commit to douglas-raillard-arm/holoviews that referenced this issue Jul 14, 2021
Take into account the backend the options are set on when creating a new
option tree. Otherwise, custom options set for a backend that is not the
current one will simply be discared, and only the new options will
survive.

Fixes holoviz#4962
@jbednar
Copy link
Member

jbednar commented Aug 3, 2021

Thanks, @philippjfr! I'd still be happy to see with hv.backend("bokeh"): appear...

@douglas-raillard-arm
Copy link
Contributor Author

@jbednar I'm happy to add that as well, which module should it live in ?

@jlstevens
Copy link
Contributor

jlstevens commented Aug 5, 2021

I'm not sure how useful a context manager like this is in a notebook context: afaik anything inside the context manager block won't automatically get displayed by the usual notebook machinery (unless you explicitly trigger display, which is awkward).

I have no objection to this idea but I would need to be shown some example use cases first to be convinced that it is useful.

@jbednar
Copy link
Member

jbednar commented Aug 5, 2021

Yes, it's very irritating that a context manager has no return value that Jupyter can display. In this case, though, the intent is to use the decorator to set options "in bulk", i.e. likely for a bunch of objects, none of which are expected to be displayed immediately. The intent is for pre-setting values for diffferent backends ahead of time, because otherwise there would be no need to use this mechanism; anything immediately displayed would only need to be supported for one backend, not flexibly switching as this context manager focuses on.

@jlstevens
Copy link
Contributor

Ok, I wouldn't anticipate this being a common need but that doesn't mean it can't be supported.

@jlstevens
Copy link
Contributor

By the way, I think as option logic this should live together with Store in core.options. It can then be exposed at a higher level though I'm not entirely sold on that just yet.

@jlstevens
Copy link
Contributor

Actually, Store is exposed at the top level and I would be happy with this context manager living there:

with hv.Store.backend('bokeh'):
   ...

@douglas-raillard-arm
Copy link
Contributor Author

douglas-raillard-arm commented Aug 5, 2021

This could probably be bundled with a way to temporarily override some global default options used when creating new plots, e.g.:

##### CODE IN A 3rd PARTY LIB #######

def lib_decorator(f):
    @functools.wraps(f)
    def wrapper(*args, **kwargs):
        # Overrides some options but otherwise inherits them from an inner context manager, or global defaults if it is the most nested level
        # "with hv.Store.options" or the global options.
        with hv.Store.options(opts.Curve(color='red'), backend='bokeh'):
            return f(*args, **kwargs)
    return wrapper

@lib_decorator
def lib_func(ys):
    return hv.Curve(ys)


# It is even possible to use the context manager as a decorator using https://docs.python.org/3/library/contextlib.html#contextlib.ContextDecorator:
theme = hv.Store.options(opts.Curve(color='red'), backend='bokeh')

@theme
def lib_func(...):
    ....
#### USER NOTEBOOK ####

opts.defaults(opts.Curve(line_dash='dashed'))

# Displayed with color=red, line_dash=dashed
lib_func([0, 1])

This can allow people to create some sort of themes, while still letting through all the global defaults that the theme does not explicitly override. The code creating the plots themselves will still be able to override the defaults where it is really necessary for e.g. readability.

A library cannot currently apply its own "local defaults" without changing the user's global defaults. With context managers, it becomes possible to have an arbitrary number of theming layers rather than just one.

Note: there seems to be a StoreOptions.options() context manager but it looks like it is limited to operating on one object, rather than the whole global defaults.

EDIT: changed the comment on how nesting of "with" should work: it makes more sense for the outer levels to override the inner levels, as the user will ultimately be in charge of the top-level.

with hv.Store.options(color='blue'):
    with hv.Store.options(color='yellow'):
        ... # color=blue here, as per the outer level. Global defaults are treated as a "virtual" innermost level, so they get the least priority. Since all that is just setting defaults, `Element.options(...)` will always be able to override any of that.

Copy link

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

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: bug Something isn't correct or isn't working
Projects
None yet
Development

No branches or pull requests

3 participants