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

[BUG] ContourColorBar update is incomplete #13841

Open
Davide-sd opened this issue Apr 26, 2024 · 3 comments
Open

[BUG] ContourColorBar update is incomplete #13841

Davide-sd opened this issue Apr 26, 2024 · 3 comments

Comments

@Davide-sd
Copy link

Software versions

Python version: 3.10.12
Bokeh: 3.4.1

Browser name and version

No response

Jupyter notebook / Jupyter Lab version

No response

Expected behavior

I expect the colorbar to always provide correct information.

Note how much code I wrote to update the colorbar. In an ideal world, I would like ContourRenderer to automatically update any colorbar associated to it. This would probably require ContourRenderer.construct_color_bar() to store the colorbar somewhere.

Also, note that I had to write levels=list(levels) inside colorbar.update. Would it be possible to get Bokeh do the casting when I provide a 1D numpy array? After all, when I created contour_renderer, the casting was managed by Bokeh...

Observed behavior

I'm trying to update a contour plot and I expect the colorbar to display correct information. Instead, as I move the sliders, the colorbar's filled colors shrinks and the tick labels don't get updated. This behavior is visible in the screenshot.

Example code

import numpy as np
from bokeh.io import output_notebook
from bokeh.layouts import column
from bokeh.models import Slider, RangeSlider, FixedTicker
from bokeh.palettes import PiYG
from bokeh.plotting import curdoc, figure, show
from bokeh.plotting.contour import contour_data
output_notebook()

def get_data(a, b, x_min, x_max, y_min, y_max):
    x, y = np.mgrid[x_min*np.pi:x_max*np.pi:100j, y_min*np.pi:y_max*np.pi:100j]
    z = (np.cos(x) + a * np.sin(x) * np.sin(y) - b * np.sin(x) * np.cos(y))**2
    return x, y, z

def update_renderers(a, b, x_min, x_max, y_min, y_max):
    x, y, z = get_data(a, b, x_min, x_max, y_min, y_max)
    levels = np.linspace(z.min(), z.max(), 10)
    print("z_max", z.max())
    new_contour_data = contour_data(x, y, z, levels)
    contour_renderer.set_data(new_contour_data)
    colorbar.update(
        levels=list(levels),
        fill_renderer=contour_renderer.fill_renderer,
        line_renderer=contour_renderer.line_renderer,
        ticker=FixedTicker(ticks=list(levels))
    )

def callback(attr, old, new):
    a = s1.value
    b = s2.value
    x_min, x_max = s3.value
    y_min, y_max = s4.value
    update_renderers(a, b, x_min, x_max, y_min, y_max)

s1 = Slider(start=0, end=2, value=1, title="a", step=0.1)
s2 = Slider(start=0, end=2, value=1, title="b", step=0.1)
s3 = RangeSlider(start=-3, end=3, value=(-1, 1), title="x_min, x_max", step=0.1)
s4 = RangeSlider(start=-3, end=3, value=(-1, 1), title="y_min, y_max", step=0.1)
[widget.on_change('value', callback) for widget in [s1, s2, s3, s4]]

fig = figure(width=600, height=450)
x, y, z = get_data(1, 1, -1, 1, -1, 1)
levels = np.linspace(z.min(), z.max(), 10)
contour_renderer = fig.contour(x, y, z, levels, fill_color=PiYG, line_color="black")
colorbar = contour_renderer.construct_color_bar()
fig.add_layout(colorbar, 'right')

layout = column([s1, s2, s3, s4, fig])

def launch_server(doc):
    doc.add_root(layout)

show(launch_server)

Stack traceback or browser console output

No response

Screenshots

image

@ianthomas23
Copy link
Member

The contouring in Bokeh is just the start of an implementation and lots of improvements need to be added. Some possible improvements are listed at #12234, this is another one.

@bryevdv
Copy link
Member

bryevdv commented Apr 26, 2024

Note how much code I wrote to update the colorbar.

FYI you wrote more code than is necessary, as there is no need to update the renderers at all. All that is needed is to update the levels and the ticker:

colorbar.update(
    levels=list(levels),
    ticker=FixedTicker(ticks=list(levels))
)

which, to be honest, does not seem like an onerous amount of code to me. If we added accepts clauses to the properties than that could probably be reduced to

colorbar.update(levels=levels, ticker=levels)

If even more simplification is desired, perhaps some new one-line convenience method colorbar.set_levels() could be added. But update is a completely generic method on the lowest base class that has a single responsibility to update individual property values, and nothing else (analogous to dict.update) We should definitely not do anything to "teach" update about any particular classes or override it to do different things on different classes.

@bryevdv
Copy link
Member

bryevdv commented Apr 26, 2024

Actually, updating the ticker object itself does not really seem to do the right thing in either case, at least on branch-3.5 But it's also not what I would advise in any case. Best practice in Bokeh is to update properties of existing objects, rather than create entire new objects (like a brand newFixedTicker). I would actually have advised writing this code:

    colorbar.levels = list(levels)
    colorbar.ticker.ticks = list(levels)

which does update the axis ticks as expected. The same potential conveniences above (and maybe others) could still make sense.

@ianthomas23 the extra space created on the top though when the ticks update, that does seem like an actual bug, do you think we should split this issue? (@Davide-sd for future ref please try to keep issues focused to single things, and especially have distinct issues for features vs bugs. This issue seemed like just a feature request at first glance, and we will definitely need to track work separately)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants