Support points 2d

[ahuang11]

Screen.Recording.2024-05-30.at.11.40.30.AM.mov

from holonote.annotate import Annotator
from holonote.app import PanelWidgets
import xarray as xr
import panel as pn
import holoviews as hv

hv.extension("bokeh")

ds = xr.tutorial.open_dataset("air_temperature")


def plot_image(time):
    image = hv.Image(ds.sel(time=time), ["lon", "lat"], ["air"]).opts(
        cmap="RdBu_r",
        title=time,
        colorbar=True,
        clim=(220, 340),
        width=500,
        height=300,
        tools=["tap"],
    )
    return image


def plot_timeseries_by_select(indices):
    if not indices:
        return

    # get the selected box
    if "start[lon]" in annotator.df.columns:
        row = annotator.df.loc[indices[0]]
        lon1 = row["start[lon]"]
        lon2 = row["end[lon]"]
        lat1 = row["start[lat]"]
        lat2 = row["end[lat]"]
        ds_sel = ds.sel(lon=slice(lon1, lon2), lat=slice(lat2, lat1)).mean(
            ["lat", "lon"]
        )
        time_series.object = hv.Curve(ds_sel["air"]).opts(title="Time Series")

    # get the selected point
    elif "point[lon]" in annotator.df.columns:
        row = annotator.df.loc[indices[0]]
        x = row["point[lon]"]
        y = row["point[lat]"]
        ds_sel = ds.sel(lon=x, lat=y, method="nearest")
        time_series.object = hv.Curve(ds_sel["air"]).opts(title="Time Series")


def plot_point_timeseries_by_stream(x, y):
    if not x:
        return

    ds_sel = ds.sel(lon=x, lat=y, method="nearest")
    time_series.object = hv.Curve(ds_sel["air"], kdims=["time"], vdims=["air"]).opts(
        title=f"Time Series {x:.2f} {y:.2f}"
    )


def plot_box_timeseries_by_stream(bounds):
    if not bounds:
        lon1, lat1, lon2, lat2 = ds.lon.min(), ds.lat.min(), ds.lon.max(), ds.lat.max()
        ds_sel = ds
    else:
        lon1, lat1, lon2, lat2 = bounds
        ds_sel = ds.sel(lon=slice(lon1, lon2), lat=slice(lat2, lat1))
    time_series.object = hv.Curve(ds_sel["air"].mean(["lat", "lon"])).opts(
        title=f"Time Series {lon1:.2f} {lat1:.2f} {lon2:.2f} {lat2:.2f}"
    )


times = ds.time.dt.strftime("%Y-%m-%d %H:%M").values.tolist()

# start annotation
annotator = Annotator(
    {"lon": float, "lat": float},
    fields=["Description", "Time", "Z"],
    default_region="point",
)
annotator_widgets = PanelWidgets(annotator, field_values={"Time": times, "Z": 100})

# make image dependent on the selected time
time_input = annotator_widgets.fields_widgets[1]
time_input.value = times[0]
image = hv.DynamicMap(pn.bind(plot_image, time_input))

time_series = pn.pane.HoloViews()

# # update plot time when a new box is SELECTED
pn.bind(plot_timeseries_by_select, annotator.param.selected_indices, watch=True)

# update plot time when a new box is CREATED
display = annotator.get_display("lat", "lon")
box_stream = display._edit_streams[0]  # to make public later
box_stream.source = image
point_stream = display._edit_streams[1]  # to make public later
point_stream.source = image
pn.bind(plot_box_timeseries_by_stream, box_stream.param.bounds, watch=True)
pn.bind(
    plot_point_timeseries_by_stream,
    point_stream.param.x,
    point_stream.param.x,
    watch=True,
)
# layout
pn.Row(annotator_widgets, pn.Column(annotator * image), time_series).show()

[codspeed-hq]

CodSpeed Performance Report

Merging #108 will not alter performance

_{Comparing add_points_2d (21e6ac5) with main (0301194)}

Summary

✅ 6 untouched benchmarks

[hoxbro]

Can you reduce your example code to show only the relevant part for this PR?

[ahuang11]

Sure

from holonote.annotate import Annotator
from holonote.app import PanelWidgets
import xarray as xr
import panel as pn
import holoviews as hv

hv.extension("bokeh")

ds = xr.tutorial.open_dataset("air_temperature")


def plot_image(time):
    image = hv.Image(ds.sel(time=time), ["lon", "lat"], ["air"]).opts(
        cmap="RdBu_r",
        title=time,
        colorbar=True,
        clim=(220, 340),
        width=500,
        height=300,
        tools=["tap"],
    )
    return image


def plot_timeseries_by_select(indices):
    if not indices:
        return
    # get the selected point
    row = annotator.df.loc[indices[0]]
    x = row["point[lon]"]
    y = row["point[lat]"]
    ds_sel = ds.sel(lon=x, lat=y, method="nearest")
    time_series.object = hv.Curve(ds_sel["air"]).opts(title="Time Series")


times = ds.time.dt.strftime("%Y-%m-%d %H:%M").values.tolist()

# start annotation
annotator = Annotator(
    {"lon": float, "lat": float},
    fields=["Description", "Time", "Z"],
    default_region="point",
)
annotator_widgets = PanelWidgets(annotator, field_values={"Time": times, "Z": 100})

# make image dependent on the selected time
time_input = annotator_widgets.fields_widgets[1]
time_input.value = times[0]
image = hv.DynamicMap(pn.bind(plot_image, time_input))
time_series = pn.pane.HoloViews()
# # update plot time when a new point is SELECTED
pn.bind(plot_timeseries_by_select, annotator.param.selected_indices, watch=True)
pn.Row(annotator_widgets, pn.Column(annotator * image), time_series).show()

[ahuang11]

Actually, at its very core:

from holonote.annotate import Annotator
from holonote.app import PanelWidgets
import xarray as xr
import panel as pn
import holoviews as hv

hv.extension("bokeh")

ds = xr.tutorial.open_dataset("air_temperature")
annotator = Annotator(
    {"lon": float, "lat": float},
    fields=["Description", "Z"],
    default_region="point",
)
annotator_widgets = PanelWidgets(annotator, field_values={"Z": 100})
image = hv.Image(ds.isel(time=0), ["lon", "lat"], ["air"]).opts(cmap="RdBu_r")
pn.Row(annotator_widgets, annotator * image).show()

[hoxbro]

I have left some comments.

I would also like to see some unit tests.

[codecov]

Codecov Report

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

Project coverage is 86.77%. Comparing base (f9deb64) to head (21e6ac5).
Report is 3 commits behind head on main.

Files	Patch %	Lines
holonote/annotate/display.py	82.35%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #108      +/-   ##
==========================================
+ Coverage   86.18%   86.77%   +0.58%     
==========================================
  Files          24       25       +1     
  Lines        2592     2662      +70     
==========================================
+ Hits         2234     2310      +76     
+ Misses        358      352       -6     

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

[hoxbro]

This prevents editing and deleting existing annotations, at least in my testing. I'm leaning toward deleting this logic.

Using your example from: #108 (comment)

[hoxbro]

After playing around with it a bit more (and also adding the style options), I can see why this is done.

I still think it is the correct thing to prioritize the simple example containing "holonote" only, over the more complex one.

Therefore, I would suggest that self._nearest_2d_point_threshold is disabled by default. Also, update the docstring with information that if the value is chosen too small, you can lose the options to edit and delete annotations.

My style diff:

diff --git a/holonote/annotate/display.py b/holonote/annotate/display.py
index 304b42b..27a96c7 100644
--- a/holonote/annotate/display.py
+++ b/holonote/annotate/display.py
@@ -88,12 +88,14 @@ class Style(param.Parameterized):
     line_opts = _StyleOpts(default={})
     span_opts = _StyleOpts(default={})
     rectangle_opts = _StyleOpts(default={})
+    points_opts = _StyleOpts(default={})
 
     # Editor opts
     edit_opts = _StyleOpts(default={"line_color": "black"})
     edit_line_opts = _StyleOpts(default={})
     edit_span_opts = _StyleOpts(default={})
     edit_rectangle_opts = _StyleOpts(default={})
+    edit_points_opts = _StyleOpts(default={})
 
     _groupby = ()
     _colormap = None
@@ -133,6 +135,7 @@ class Style(param.Parameterized):
             hv.opts.HSpans(**opts, **self.span_opts),
             hv.opts.VLines(**opts, **self.line_opts),
             hv.opts.HLines(**opts, **self.line_opts),
+            hv.opts.Points(**opts, **self.points_opts),
         )
 
     def editor(self) -> tuple[hv.Options, ...]:
@@ -148,6 +151,7 @@ class Style(param.Parameterized):
             hv.opts.HSpan(**opts, **self.edit_span_opts),
             hv.opts.VLine(**opts, **self.edit_line_opts),
             hv.opts.HLine(**opts, **self.edit_line_opts),
+            hv.opts.Points(**opts, **self.points_opts),
         )
 

[ahuang11]

Thanks for reviewing. I believe the last line of the opts should be edit_points_opts? If so, I've implemented that.

Therefore, I would suggest that self._nearest_2d_point_threshold is disabled by default

I disagree; without it, users click anywhere, and it selects and highlights the nearest existing point, but that can be misleading the users because they see the previous point highlighted instead of where they clicked, and that's no where near what they want.

Screen.Recording.2024-07-01.at.12.29.04.PM.mov

Based on these comments:

This prevents editing and deleting existing annotations, at least in my testing.

I still think it is the correct thing to prioritize the simple example containing "holonote" only, over the more complex one.

We can just up the threshold to 1, and it works fine.

Screen.Recording.2024-07-01.at.12.29.51.PM.mov

[hoxbro]

I believe the last line of the opts should be edit_points_opts?

Correct. Copy / paste error.

We can just up the threshold to 1, and it works fine.

It works "fine" with the current magnitude of lon/lat. If we edit this, we will see the same problems.

factor = 1e3  # or 1e-3
ds["lon"] = ds["lon"] * factor
ds["lat"] = ds["lat"] * factor

It can be hard to hit on a larger scale. On a smaller scale, we have the same problem as without the limit. So, having it enabled by default, no matter the value, we end up in a Goldilocks situation where it only works if it is just right and creates confusing behavior if it is off.

large.mp4

small.mp4

previous point highlighted instead of where they clicked

This is somewhat improved with the updated style, which highlights the selected annotation.

[ahuang11]

On a smaller scale, we have the same problem as without the limit. So, having it enabled by default, no matter the value, we end up in a Goldilocks situation where it only works if it is just right and creates confusing behavior if it is off.

Which is why I made it a public param, similar to np.testing.assert_almost_equal. Alternatively, we could get the order of magnitude of the ranges in the dataset and use that.

This is somewhat improved with the updated style, which highlights the selected annotation.

To clarify, I meant while creating a new point, not editing an existing.