-
-
Notifications
You must be signed in to change notification settings - Fork 111
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
refactored getting started guide #1476
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for the latest changes after our informal PR review!
This PR removes some pages which means it breaks links to these pages. We use sphinx-rediraffe
to add redirect links when things like that happen, you can find how it's configured in conf.py
with the rediraffe_redirects
dictionary. For example, users trying to reach the removed interactive page should be redirected to the refactored getting started (I don't know if we can/should redirect to the specific Interactive fragment of the refactored getting started).
In fact we'll have to touch these redirects again soon since we're planning to move the Getting Started under a new top-level Tutorials sections. Nevertheless, I think it's worth doing it here, as we need to always make sure we always add redirect links when we move/remove a page.
doc/getting_started/index.ipynb
Outdated
"outputs": [], | ||
"source": [ | ||
"import cartopy.crs as crs\n", | ||
"\n", | ||
"proj = crs.Orthographic(-90, 30)\n", | ||
"air_ds.air.sel(time='2013-06-01 12:00').hvplot.quadmesh(\n", | ||
" 'lon', 'lat', projection=proj, project=True,\n", | ||
" global_extent=True, cmap='viridis', coastline=True\n", | ||
" 'lon', 'lat', projection=proj, global_extent=True,\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.
Today we discussed starting the Geographic section with some simple example that doesn't leverage GeoViews and displays lat/lon data over web map tiles. Have you considered adding this?
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.
Thanks for reminding me. i think I forgot what exactly I was supposed to do there.
Here's the current code:
air_ds.air.sel(time='2013-06-01 12:00').hvplot.quadmesh( 'lon', 'lat', tiles=True,
cmap='viridis', yaxis=None, xaxis=None,
)
Image:
Does this work @maximlt ?
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.
Sorry for the late reply, I was more thinking of a more simpler points plot (e.g. location of cities). But yes, the idea is not to use geo=True
!
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.
Ok in fact I was thinking of not removing the original example but adding first a simpler example that doesn't require GeoViews. Basically, I don't want users to think they need always GeoViews to display geographic data, and show them when that's possible.
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.
doc/getting_started/index.ipynb
Outdated
"import xarray as xr\n", | ||
"\n", | ||
"air_ds = xr.tutorial.open_dataset('air_temperature').load()\n", | ||
"air_ds['air'].isel(time=slice(0, 1000, 60)).hvplot.image(dynamic=False, width=400, height=300)" |
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.
Can you check how the plots that set width=...
look like on a smartphone screen? I'd be great if you at least make the Getting Started look good on smartphones.
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.
For the plots that have a widget (e.g, using groupby
), setting responsive=True
was not good on smartphone screens as the widgets maintained their original size while the plots resized. This made the widget to cut across the whole plot. So I maintained the set widths.
For others, I set responsive=True
and it was OK on both laptop and phone screens.
However, the explorer
plot did not look good on a smartphone screen whether set to responsive or not.
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.
This made the widget to cut across the whole plot
Ugh! Can you file an issue for this? That's concerning.
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.
However, the explorer plot did not look good on a smartphone screen whether set to responsive or not.
Yep not too surprised.
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.
Filed the Issue #1484
"id": "c226b5d7-8608-4998-b34a-064f7b21135c", | ||
"metadata": {}, | ||
"source": [ | ||
"**Without GeoViews**" |
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.
It'd be nice to quickly mention when geographic data can be displayed on tiles without having to install geoviews, which is when the data is in lat/long coordinates.
fixes #1473