-
-
Notifications
You must be signed in to change notification settings - Fork 105
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
Automatically convert lonlat to xy when tiles=True #1377
Conversation
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])
I believe it matches what Jim proposed:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
andy
? - 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 anhv.Dataset
to update the element's_dataset
attribute withobj._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 setgeo=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 justtiles=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 withprojection=False
.
I'm not sure it's useful in this case; it creates a histogram if 3D
Haha I love that there's both a # 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
I support this idea; it's a better default than having the user to figure out to use tiles, additionally need |
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] |
@@ -415,7 +417,7 @@ | |||
"- `global_extent` (default=False): Whether to expand the plot extent to span the whole globe\n", | |||
"- `project` (default=False): Whether to project the data before plotting (adds initial overhead but avoids projecting data when plot is dynamically updated)\n", | |||
"- `projection` (default=None): Coordinate reference system of the plot (output projection) specified as a string or integer EPSG code, a CRS or Proj pyproj object, a Cartopy CRS object or class name, a WKT string, or a proj.4 string. Defaults to PlateCarree.\n", | |||
"- `tiles` (default=False): Whether to overlay the plot on a tile source. Accept the following values:\n", | |||
"- `tiles` (default=False): Whether to overlay the plot on a tile source. If coordinate values fall within lat/lon bounds, auto-projects to EPSG:3857 unless `projection=False`. Accepts the following values:\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on removing this cell and only link directly to Customization geographic section to reduce redundancy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to keep them here for now, I think it's nice to have all the options there within the appropriate context.
Codecov ReportAttention: Patch coverage is
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. |
I'm happy with the proposal to project only if |
@@ -415,7 +417,7 @@ | |||
"- `global_extent` (default=False): Whether to expand the plot extent to span the whole globe\n", | |||
"- `project` (default=False): Whether to project the data before plotting (adds initial overhead but avoids projecting data when plot is dynamically updated)\n", | |||
"- `projection` (default=None): Coordinate reference system of the plot (output projection) specified as a string or integer EPSG code, a CRS or Proj pyproj object, a Cartopy CRS object or class name, a WKT string, or a proj.4 string. Defaults to PlateCarree.\n", | |||
"- `tiles` (default=False): Whether to overlay the plot on a tile source. Accept the following values:\n", | |||
"- `tiles` (default=False): Whether to overlay the plot on a tile source. If coordinate values fall within lat/lon bounds, auto-projects to EPSG:3857 unless `projection=False`. Accepts the following values:\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to keep them here for now, I think it's nice to have all the options there within the appropriate context.
|
||
if is_geodataframe(data): | ||
if data.crs is not None: | ||
data = data.to_crs(epsg=3857) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Closes #1375
Will add tests next week if this logic is ok.
Basically if user sets
geo=True
without crs/projection, it does apseudo-geo
(i.e. perform projection and toggle off)