Automatically convert lonlat to xy when tiles=True

[ahuang11]

Closes #1375

Will add tests next week if this logic is ok.

Basically if user sets geo=True without crs/projection, it does a pseudo-geo (i.e. perform projection and toggle off)

[maximlt]

@ahuang11 you assigned me as a reviewer but this is still marked as Draft and the code seems to differ from what was discussed in #1375 (no warning, automatic conversion). So I'm not sure I should start to review yet?

[ahuang11]

you assigned me as a reviewer but this is still marked as Draft

Will add tests next week if this logic is ok.

Wanted to first see if you're okay with it because of

            min_x = np.min(data[x])
            max_x = np.max(data[x])
            min_y = np.min(data[y])
            max_y = np.max(data[y])

seems to differ from what was discussed in #1375 (no warning, automatic conversion).

I believe it matches what Jim proposed:

I'd be ok with assuming lat,lon inputs with geo=True and projecting them using our own internal functions from HoloViews without having to bring in geoviews/cartopy/proj for that one common case.

[maximlt]

Overall I'm very supportive of this idea! Happy we're working on making it easier to display lat/lon data. It would have made updating some of the HoloViz examples even easier, for instance https://examples.holoviz.org/gallery/gull_tracking/gull_tracking.html.

(And that's for a future iteration but I'm sure we can also streamline plotting/converting GeoDataFrame objects)

---

I believe it matches what Jim proposed

Yep true sorry, I got confused by his second sentence, since in the PR the ranges are checked, except that it's done only when geo=True so that seems to fit with what he described. Pinging him to get his feedback anyway since he had opinions @jbednar.

I'd be ok with assuming lat,lon inputs with geo=True and projecting them using our own internal functions from HoloViews without having to bring in geoviews/cartopy/proj for that one common case. I would not be ok with checking the ranges and automatically converting; at best that could be a warning (maybe people really do want to look at some tiny area of the earth, or vice versa!).

---

Wanted to first see if you're okay with it because of

I'm ok with how the ranges are computed. However, I'm not sure this should be computed there, right at the beginning of the converters' __init__ method?

• Isn't there code that automatically infer x and y?
• There's self.source_data = data further down the processing pipeline that is a handle on the original data. It is later used to create an hv.Dataset to update the element's _dataset attribute with obj._dataset = dataset. Unfortunately, I know that removing this line doesn't break any unit test. I vaguely remember @philippjfr mentioned it had something to do with streams. So I'd recommend checking with him where the conversion should take place, and if this going to affect that part of the code.

---

Basically if user sets geo=True without crs/projection

Cannot we enable the automatic conversion when users set tiles=True, without requiring geo=True?

• In my mental model, geo=True means that GeoViews will be involved, so this change as is would divert from that.
• Users who set tiles=True want a geo-plot by definition. We let them not set geo=True, and don't set it internally, as a way to avoid bringing in GeoViews. So we've already set a precedent there, and I think we should pursue in the same direction with checking the ranges and converting when just tiles=True is passed (so that's against what @jbednar said). For the small proportion of people that will want to look at that tiny area of the earth, they could disable it with projection=False.

[ahuang11]

Isn't there code that automatically infer x and y?

I'm not sure it's useful in this case; it creates a histogram if 3D

There's self.source_data = data further down the processing pipeline that is a handle on the original data.

Haha I love that there's both a self.data_source and self.source_data.

        # Validate DataSource
        self.data_source = data

        self.is_series = is_series(data)
        if self.is_series:
            data = data.to_frame()
        if is_intake(data):
            data = process_intake(data, use_dask or persist)
        # Pandas interface in HoloViews doesn't accept non-string columns.
        # The converter stores a reference to the source data to
        # update the `_dataset` property (of the hv object its __call__ method
        # returns) with a hv Dataset created from the source data, which
        # is done for optimizating some operations in HoloViews.
        data = _convert_col_names_to_str(data)

        self.source_data = data

Users who set tiles=True want a geo-plot by definition. We let them not set geo=True, and don't set it internally, as a way to avoid bringing in GeoViews. So we've already set a precedent there, and I think we should pursue in the same direction with checking the ranges and converting when just tiles=True is passed (so that's against what @jbednar said). For the small proportion of people that will want to look at that tiny area of the earth, they could disable it with projection=False.

I support this idea; it's a better default than having the user to figure out to use tiles, additionally need geo=True.

[ahuang11]

Migrated to after inferring x/y, but not sure what to do with self.source_data/self.data_source, i.e. should I update those too?

Wondering if I should rename x,y.

Separately, I think hover needs fixing, but I think it's better for it to be implemented in HoloViews; adapted from GeoViews https://github.com/holoviz/geoviews/blob/8a14f9c71298c86e1b364c18ac952bf310e002ac/geoviews/plotting/bokeh/__init__.py#L162

            from bokeh.models import HoverTool
            from bokeh.models import CustomJSHover
            
            hover = HoverTool()
            xdim = x if x in data else self.x
            ydim = y if y in data else self.y
            formatters, tooltips = dict(hover.formatters), []
            xhover = CustomJSHover(code=_hover_code % 0)
            yhover = CustomJSHover(code=_hover_code % 1)
            for name, formatter in hover.tooltips:
                customjs = None
                if formatter in (f'@{{{xdim}}}', '$x'):
                    dim = xdim
                    formatter = '$x'
                    customjs = xhover
                elif formatter in (f'@{{{ydim}}}', '$y'):
                    dim = ydim
                    formatter = '$y'
                    customjs = yhover
                if customjs:
                    key = formatter if formatter in ('$x', '$y') else dim
                    formatters[key] = customjs
                    formatter += '{custom}'
                tooltips.append((name, formatter))
            hover.tooltips = tooltips
            hover.formatters = formatters
            self._plot_opts['tools'] = [hover]

[ahuang11]

import xarray as xr
import hvplot.xarray
import holoviews as hv
import panel as pn

ds = xr.tutorial.open_dataset("air_temperature")
ds.hvplot(tiles=True, groupby=["time"])

import geopandas as gpd
import hvplot.pandas
from geodatasets import get_path

path = get_path("geoda.natregimes")

gdf = gpd.read_file(path)
gdf.hvplot(tiles=True)

!wget https://s3.eu-west-1.amazonaws.com/datasets.holoviz.org/HG_OOSTENDE/v1/HG_OOSTENDE-gps-2018.csv

import pandas as pd
import hvplot.pandas 

df = pd.read_csv("HG_OOSTENDE-gps-2018.csv")
df.hvplot.points("location-long", "location-lat", tiles=True, datashade=True)

[ahuang11]

Thoughts on removing this cell and only link directly to Customization geographic section to reduce redundancy?

[maximlt]

I prefer to keep them here for now, I think it's nice to have all the options there within the appropriate context.

[codecov]

Codecov Report

Attention: Patch coverage is 97.77778% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.89%. Comparing base (6c96c7e) to head (732389e).
Report is 23 commits behind head on main.

Files with missing lines	Patch %	Lines
hvplot/converter.py	95.83%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1377      +/-   ##
==========================================
+ Coverage   87.39%   88.89%   +1.50%     
==========================================
  Files          50       51       +1     
  Lines        7490     7531      +41     
==========================================
+ Hits         6546     6695     +149     
+ Misses        944      836     -108     

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

[jbednar]

I'm happy with the proposal to project only if tiles is True, not geo; this projection is explicitly about getting things in shape for Bokeh using Web Mercator, and not just generally when working with geo data (where the data can be left unprojected).

[maximlt]

I prefer to keep them here for now, I think it's nice to have all the options there within the appropriate context.

[maximlt]

Nice!