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

Expose setting hard navigable bounds #6056

Merged
merged 24 commits into from
Apr 16, 2024
Merged

Expose setting hard navigable bounds #6056

merged 24 commits into from
Apr 16, 2024

Conversation

droumis
Copy link
Member

@droumis droumis commented Jan 1, 2024

This PR closes #1019, which pertains to the lack of hard bounds when panning and zooming using the Bokeh backend.

Why? Previously, the axes were unbounded, allowing users to zoom and pan to potentially empty parts of the data space. This is particularly annoying when a plot captures the scroll of a webpage/notebook/app that it is a part of. This PR changes the default behavior adds a flag, preventing zooming or panning beyond the data range. update: uses the more extreme of data ranges or xlim/ylim. Update, if dim ranges are set, they are used as hard bounds.

To enable this new behavior, use apply_hard_bounds=True.

Limitation: Currently, setting hard navigable bounds with Bokeh means that if you hit a single extent (e.g. the lower x dim), the zoom won't continue to work for the remaining directions (upper x dim, or any y dim).. Bokeh has an option called maintain_focus=False for zoom tools to enable continued zoom. Future work could potentially enable this option in HoloViews when apply_hard_bounds=True.

Todo:

  • add tests
  • add tests for datetime axes
  • add test for dmap updates
  • add docs

Demo:

code
hb_default = hv.Curve(([1,2,3], [1,2,1]), label='apply hard bounds Default')
hb_false = hv.Curve(([1,2,3], [4,2,3]), label='apply hard bounds False').opts(apply_hard_bounds=False, color='black')
hb_true = hv.Curve(([-1,0,1,2,3,4], [3,2,1,0,-1,-2]), label='apply hard bounds True').opts(apply_hard_bounds=True, color='red')
hb_overlay = (hb_true * hb_false).opts(title='True/False overlay', show_legend=False)

(hb_default + hb_false + hb_true + hb_overlay).opts(shared_axes=False)
Screen.Recording.2024-04-15.at.11.40.17.AM.mov

Only limits the x dim for subcoord_y plots:

code
import numpy as np
import holoviews as hv
from bokeh.models import HoverTool
from holoviews.plotting.links import RangeToolLink
from scipy.stats import zscore

hv.extension('bokeh')

N_CHANNELS = 10
N_SECONDS = 5
SAMPLING_RATE = 200
INIT_FREQ = 2  # Initial frequency in Hz
FREQ_INC = 5  # Frequency increment
AMPLITUDE = 1

# Generate time and channel labels
total_samples = N_SECONDS * SAMPLING_RATE
time = np.linspace(0, N_SECONDS, total_samples)
channels = [f'EEG {i}' for i in range(N_CHANNELS)]

# Generate sine wave data
data = np.array([AMPLITUDE * np.sin(2 * np.pi * (INIT_FREQ + i * FREQ_INC) * time)
                     for i in range(N_CHANNELS)])

hover = HoverTool(tooltips=[
    ("Channel", "@channel"),
    ("Time", "$x s"),
    ("Amplitude", "$y µV")
])

channel_curves = []
for channel, channel_data in zip(channels, data):
    ds = hv.Dataset((time, channel_data, channel), ["Time", "Amplitude", "channel"])
    curve = hv.Curve(ds, "Time", ["Amplitude", "channel"], label=channel)
    curve.opts(
        subcoordinate_y=True, color="black", line_width=1, tools=[hover],
        apply_hard_bounds=True,
    )
    channel_curves.append(curve)

eeg = hv.Overlay(channel_curves, kdims="Channel").opts(
    xlabel="Time (s)", ylabel="Channel", show_legend=False, aspect=3, responsive=True,
)


y_positions = range(N_CHANNELS)
yticks = [(i , ich) for i, ich in enumerate(channels)]

z_data = zscore(data, axis=1)

minimap = hv.Image((time, y_positions , z_data), ["Time (s)", "Channel"], "Amplitude (uV)")
minimap = minimap.opts(
    cmap="RdBu_r", xlabel='Time (s)', alpha=.5, yticks=[yticks[0], yticks[-1]],
    height=150, responsive=True, default_tools=[], clim=(-z_data.std(), z_data.std())
)


RangeToolLink(
    minimap, eeg, axes=["x", "y"],
    boundsx=(None, 2), boundsy=(None, 6.5)
)

dashboard = (eeg + minimap).opts(merge_tools=False).cols(1)
dashboard
Screen.Recording.2024-04-15.at.11.29.52.AM.mov

@droumis droumis added status: WIP tag: backend: bokeh type: enhancement Minor feature or improvement to an existing feature czi labels Jan 1, 2024
@droumis
Copy link
Member Author

droumis commented Jan 2, 2024

I was getting some test errors when an element didn't have an extents attribute, so I'm skipping when this is the case. I'm assuming hard bounds are irrelevant for such objects.

@droumis
Copy link
Member Author

droumis commented Jan 2, 2024

I'm also setting bounds to None for cases where the element.extents are identical (e.g. (10.0, 10.0)) to avoid some test errors like:

example test error:

____________________ TestPointPlot.test_op_ndoverlay_value _____________________

self = <holoviews.tests.plotting.bokeh.test_pointplot.TestPointPlot testMethod=test_op_ndoverlay_value>

    def test_op_ndoverlay_value(self):
        markers = ['circle', 'triangle']
        overlay = NdOverlay({marker: Points(np.arange(i)) for i, marker in enumerate(markers)}, 'Marker').opts('Points', marker='Marker')
>       plot = bokeh_renderer.get_plot(overlay)

holoviews/tests/plotting/bokeh/test_pointplot.py:503:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
holoviews/plotting/bokeh/renderer.py:68: in get_plot
    plot = super().get_plot(obj, doc, renderer, **kwargs)
holoviews/plotting/renderer.py:240: in get_plot
    plot.update(init_key)
holoviews/plotting/plot.py:955: in update
    return self.initialize_plot()
holoviews/plotting/bokeh/element.py:2930: in initialize_plot
    child = subplot.initialize_plot(ranges, plot, plots)
holoviews/plotting/bokeh/element.py:1890: in initialize_plot
    self._apply_hard_bound(element, ranges)
holoviews/plotting/bokeh/element.py:1937: in _apply_hard_bound
    set_bounds('x_range', xmin, xmax)
holoviews/plotting/bokeh/element.py:1935: in set_bounds
    self.handles[axis].bounds = (min_bound, max_bound) if min_bound is not None or max_bound is not None else None
../../opt/miniconda3/envs/hv-dev-tests/lib/python3.9/site-packages/bokeh/core/has_props.py:334: in __setattr__
    return super().__setattr__(name, value)
../../opt/miniconda3/envs/hv-dev-tests/lib/python3.9/site-packages/bokeh/core/property/descriptors.py:331: in __set__
    value = self.property.prepare_value(obj, self.name, value)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = Nullable(MinMaxBounds(Auto, Tuple(Float, Float), Tuple(Nullable(Float), Float), Tuple(Float, Nullable(Float)), Tuple(T...able(TimeDelta)), Tuple(Datetime, Datetime), Tuple(Nullable(Datetime), Datetime), Tuple(Datetime, Nullable(Datetime))))
owner = Range1d(id='p42539', ...), name = 'bounds', value = (0.0, 0.0)

    def prepare_value(self, owner: HasProps | type[HasProps], name: str, value: Any, *, hint: DocumentPatchedEvent | None = None) -> T:
        if value is Intrinsic:
            value = self._raw_default()
        if value is Undefined:
            return value

        error = None
        try:
            if validation_on():
                hinted_value = self._hinted_value(value, hint)
                self.validate(hinted_value)
        except ValueError as e:
            for tp, converter in self.alternatives:
                if tp.is_valid(value):
                    value = converter(value)
                    break
            else:
                error = e

        if error is None:
            value = self.transform(value)
        else:
            obj_repr = owner if isinstance(owner, HasProps) else owner.__name__
>           raise ValueError(f"failed to validate {obj_repr}.{name}: {error}")
E           ValueError: failed to validate Range1d(id='p42539', ...).bounds: expected either None or a value of type MinMaxBounds(Auto, Tuple(Float, Float), Tuple(Nullable(Float), Float), Tuple(Float, Nullable(Float)), Tuple(TimeDelta, TimeDelta), Tuple(Nullable(TimeDelta), TimeDelta), Tuple(TimeDelta, Nullable(TimeDelta)), Tuple(Datetime, Datetime), Tuple(Nullable(Datetime), Datetime), Tuple(Datetime, Nullable(Datetime))), got (0.0, 0.0)

../../opt/miniconda3/envs/hv-dev-tests/lib/python3.9/site-packages/bokeh/core/property/bases.py:365: ValueError

@droumis droumis marked this pull request as ready for review January 2, 2024 01:27
@codecov-commenter
Copy link

codecov-commenter commented Jan 2, 2024

Codecov Report

Attention: Patch coverage is 96.62921% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 88.50%. Comparing base (bf4592e) to head (4056f6e).
Report is 7 commits behind head on main.

❗ Current head 4056f6e differs from pull request most recent head ca106d1. Consider uploading reports for the commit ca106d1 to get more accurate results

Files Patch % Lines
holoviews/tests/ui/bokeh/test_callback.py 0.00% 2 Missing ⚠️
holoviews/plotting/bokeh/element.py 93.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6056      +/-   ##
==========================================
- Coverage   88.70%   88.50%   -0.20%     
==========================================
  Files         316      317       +1     
  Lines       66121    66238     +117     
==========================================
- Hits        58650    58627      -23     
- Misses       7471     7611     +140     
Flag Coverage Δ
ui-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@droumis
Copy link
Member Author

droumis commented Jan 3, 2024

I disabled hard bounds for one of the UI tests (test_multi_axis_rangexy) in order to have it pass.

Copy link
Member

@philippjfr philippjfr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a few unit tests to make sure that:

  1. The bounds are correctly set for both numeric and datetime axes
  2. The bounds update when the element updates

Copy link
Member

@philippjfr philippjfr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more comment, it's not 100% clear to me if the hard bounds should always be drawn from the combined bounds, i.e. data range, Dimension.range, Dimension.soft_range and {x|ylim or whether that behavior should be configurable.

@droumis droumis changed the title Apply hard navigable bounds by default Expose setting hard navigable bounds Apr 5, 2024
@droumis droumis marked this pull request as draft April 9, 2024 01:30
@droumis
Copy link
Member Author

droumis commented Apr 9, 2024

Several issues remain.

We want it to use the largest of data + padding, xlim/ylim, dimension ranges, but it is not yet achieving that.

This also does not seem to work for y-axis with subcoordinate_y plots

@droumis
Copy link
Member Author

droumis commented Apr 12, 2024

We want it to use the largest of data + padding, xlim/ylim, dimension ranges, but it is not yet achieving that.

We determined that dimension ranges are a special, strict case and that a more semantically accurate handling of them includes using them as absolute hard bounds whenever specified, and when apply_hard_bounds=True.

So the current plan when apply_hard_bounds=True:

  • If dimension ranges are set (e.g. via.redim.range), allow navigation only up to these bounds, regardless of whether the data extends beyond.
  • Otherwise, use the more extreme of either the data+padding ranges or, if specified, xlim/ylim ranges.

@droumis
Copy link
Member Author

droumis commented Apr 12, 2024

This also does not seem to work for y-axis with subcoordinate_y plots

We determined that the it's best if the y-axis for subcoordinate_y plots remain unconstrained in all cases, so this bug is now a feature.

@droumis
Copy link
Member Author

droumis commented Apr 12, 2024

One more comment, it's not 100% clear to me if the hard bounds should always be drawn from the combined bounds, i.e. data range, Dimension.range, Dimension.soft_range and x|ylim or whether that behavior should be configurable.

I think further configurability can be future improvements based on user feedback. This could also include which, if not all, dims the hard bounds should apply to.

@droumis
Copy link
Member Author

droumis commented Apr 12, 2024

Also need to test overlaid plots, especially when one or both have dim ranges set..

@droumis droumis marked this pull request as ready for review April 15, 2024 18:45
@droumis
Copy link
Member Author

droumis commented Apr 15, 2024

ready for review

Copy link
Member

@hoxbro hoxbro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have left some small comments, but other than that, this looks good to me.

holoviews/tests/ui/bokeh/test_callback.py Outdated Show resolved Hide resolved
@@ -1423,7 +1423,7 @@ def _get_range_extents(self, element, ranges, range_type, xdim, ydim, zdim):

return (x0, y0, x1, y1)

def get_extents(self, element, ranges, range_type='combined', dimension=None, xdim=None, ydim=None, zdim=None, **kwargs):
def get_extents(self, element, ranges, range_type='combined', dimension=None, xdim=None, ydim=None, zdim=None, lims_as_soft_ranges=False, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there other inherited classes that need lims_as_soft_ranges?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm are you hinting at the get_extents method of the class GenericOverlayPlot?

It doesn't appear to be necessary because even for overlay plots, GenericElementPlot's version of get_extents with lims_as_soft_ranges set to True seems to be called anyway. But honestly it's all pretty confusing and get_extents gets called numerous times so I could be missing something.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not hinting at anything specific; just that other classes could also need to implement lims_as_soft_ranges if they don't have the super. But I think we should wait and see if there is a demand for it, before doing anything.

holoviews/plotting/bokeh/element.py Outdated Show resolved Hide resolved
holoviews/plotting/bokeh/element.py Outdated Show resolved Hide resolved
holoviews/tests/plotting/bokeh/test_elementplot.py Outdated Show resolved Hide resolved
droumis and others added 3 commits April 16, 2024 09:45
Co-authored-by: Simon Høxbro Hansen <simon.hansen@me.com>
Co-authored-by: Simon Høxbro Hansen <simon.hansen@me.com>
@droumis
Copy link
Member Author

droumis commented Apr 16, 2024

observation, bar charts are only constrained on the continuous axis. I think that's ok for now. see bokeh/bokeh#13830

data = [('one',8),('two', 10), ('three', 16), ('four', 8), ('five', 4), ('six', 1)]
bars = hv.Bars(data, hv.Dimension('Car occupants'), 'Count').opts(apply_hard_bounds=True)
bars
Screen.Recording.2024-04-16.at.10.36.37.AM.mov

@droumis
Copy link
Member Author

droumis commented Apr 16, 2024

observation: adding vspans to a plot that already has apply_hard_bounds=True doesn't screw up the hard range. ✅

Also, setting apply_hard_bounds=True directly on the vspan/hspan element is pointless as they are funtionaly unbounded.

image

@droumis droumis merged commit ffb1293 into main Apr 16, 2024
11 checks passed
@droumis droumis deleted the hard-bounds branch April 16, 2024 21:03
@droumis droumis self-assigned this Apr 16, 2024
@jbednar
Copy link
Member

jbednar commented Apr 17, 2024

bar charts are only constrained on the continuous axis

Is it possible to disable zooming and panning on the non-continous axis of a bar chart? All it ever seems to do is screw up the plot (as in the gif above); I don't recall ever wanting to do it deliberately and it very often messes up the plot as I'm working with it.

@droumis
Copy link
Member Author

droumis commented Apr 17, 2024

not declaratively, but you can do something like this if you know which axis is going to be non-continuous:

default_tools=['ywheel_zoom', 'ypan', 'reset']

Screen.Recording.2024-04-17.at.7.57.10.AM.mov

Copy link

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.

@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
czi tag: backend: bokeh type: enhancement Minor feature or improvement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Defining hard bounds when panning and zooming in bokeh backend
5 participants