-
Notifications
You must be signed in to change notification settings - Fork 224
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
Add function to load raster tile maps using contextily #2125
Conversation
New dataset function to load XYZ tiles! Uses contextily to retrieve the tiles based on a bounding box. Included an example doctest that shows how the map tiles can be loaded into an xarray.DataArray. Added a new section in the API docs and intersphinx mappings for contextily and xyzservices.
Can't assume that the input bounding box (which can be in longitude/latitude) is the same as the returned extent (which is always in EPSG:3857).
To fix pylint `C0103: Argument name "ll" doesn't conform to snake_case naming style (invalid-name)`.
Remove the extent and _image variables to prevent `R0914: Too many local variables (17/15) (too-many-locals)`.
Let the Continuous Integration tests run with `contextily`, include it in pyproject.toml and environment.yml, and document it in `doc/install.rst` as an optional dependency.
Bounding box coordinates are assumed to be longitude/latitude by default, rather than in Spherical Mercator.
Using the `__doctest_requires__` variable, see https://github.com/astropy/pytest-doctestplus/tree/v0.12.1#doctest-dependencies
pygmt/datasets/map_tiles.py
Outdated
import numpy as np | ||
import xarray as xr | ||
|
||
__doctest_requires__ = {("load_map_tiles"): ["contextily"]} |
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.
Found a nice way to skip the doctest when contextily
is not installed (e.g. in the Python 3.8 / NumPy 1.21
tests) using https://github.com/astropy/pytest-doctestplus/tree/v0.12.1#doctest-dependencies. Thanks to #1790 for adding pytest-doctestplus
!
Co-Authored-By: Dongdong Tian <[email protected]>
Also updated intersphinx link for xarray to new URL at https://docs.xarray.dev/en/stable
This PR was bumped back to the 0.9.0 milestone by @seisman two weeks ago. But @weiji14 you added this PR to the list of priority issues/PRs for release v.0.8.0 some days ago. Currently, this PR is still in draft status and there are some conflicts with the main branch. Do you feel you can finish this PR and there is enough time for reviewers to look at it 🙂? |
Thanks for the ping. I've resolved the conflicts and removed the draft status. No strong opinion on whether to have it in v0.8.0 or v0.9.0, if it takes too long to review, we can merge it next year (2023). |
Hm. As this PR was in draft status, I thought you plan more changes here. But if it is already ready for review let's see, if people |
pygmt/datasets/tile_map.py
Outdated
source : xyzservices.TileProvider or str | ||
Optional. The tile source: web tile provider or path to a local file. | ||
The web tile provider can be in the form of a | ||
:class:`xyzservices.TileProvider` object or a URL. The placeholders for | ||
the XYZ in the URL need to be {x}, {y}, {z}, respectively. For local | ||
file paths, the file is read with :doc:`rasterio <rasterio:index>` and | ||
all bands are loaded into the basemap. IMPORTANT: tiles are assumed to | ||
be in the Spherical Mercator projection (EPSG:3857). [Default is | ||
``xyzservices.providers.Stamen.Terrain``, i.e. Stamen Terrain web | ||
tiles]. |
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.
Currently, xyzservices.TileProvider
is linked to its API documentation (https://xyzservices.readthedocs.io/en/stable/api.html#xyzservices.TileProvider) which is very technical.
Perhaps we should add a link to https://contextily.readthedocs.io/en/latest/intro_guide.html#Providers so that users can quickly see the list of all available providers?
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, I've linked to https://contextily.readthedocs.io/en/stable/providers_deepdive.html which seems a bit better (actually mentioned before in #2125 (comment)) and split out the three possible source
options as bullet points. Done in commit aa2d5fb. Take a look to see if it's ok.
Split out the three possible source options into bullet points. Link to https://contextily.readthedocs.io/en/latest/providers_deepdive.html, give an example OpenStreetMap URL, and link to https://contextily.readthedocs.io/en/stable/working_with_local_files.html.
Even with the option to reproject to EPSG:4326, I still think the default should be EPSG:3857 since most web tiles are served using EPSG:3857 as mentioned in #2125 (comment). We shouldn't be reprojecting (and causing a distortion in the pixels) by default to EPSG:4326. |
Will be a great improvement! |
Co-authored-by: Michael Grund <[email protected]>
Thanks everyone! I'll leave this open for another day in case there's anything else. |
pygmt/datasets/tile_map.py
Outdated
zoom : int | ||
Optional. Level of detail. [Default is 'auto']. |
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.
Hm, from the default it seems like that there is also string input possible.
Currently, I am wondering what "level of detail" means. Do users have to pass a percentage value to zoom
? Or is zoom
something similar we have for rivers
of pygmt.Figure.coast
? Maybe we should also state what "auto"
actually means?
zoom : int | |
Optional. Level of detail. [Default is 'auto']. | |
zoom : int or str | |
Optional. Level of detail [Default is ``"auto"``]. |
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.
Good catch. Yes, I did wonder if people actually knew what to use for zoom
! The zoom
here refers to an integer from 0 to 22, where 0 is the most zoomed out (i.e. the whole globe), and 22 is the most zoomed in (i.e. you can see buildings. See https://docs.mapbox.com/help/glossary/zoom-level or https://leafletjs.com/examples/zoom-levels for a good definition.
There is a small complication in that not all tile sources supports zoom up to level 22, so it's a bit hard to just document something like "Use an integer between 0 and 22 (inclusive)". But let me try adding a better description of this zoom
parameter.
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, more detail added in 9f14d31. Let me know if that reads ok!
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.
Agree that the description of zoom
should be expanded a little bit since in my opinion this is a great (GMT-) improvement for people doing work on small scale levels such as cities or communities and want to have geographic content (streets, buidldings, parks etc.) in the background as reference. I missed such things in the past and used other libraries or created customized stuff. Definitely a feature we should highlight in the next release! Great work @weiji14 😉
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 @weiji14 for expanding the docstring for zoom
! Now it is clearer how to use this parameter 🙂.
Co-Authored-By: Yvonne Fröhlich <[email protected]>
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.
The description of zoom
should be sufficient to understand what the parameter does.
Co-authored-by: Michael Grund <[email protected]>
Co-authored-by: Yvonne Fröhlich <[email protected]>
The doctest fails for the CI run with Windows + Python 3.11 + NumPy 1.24 (see https://github.com/GenericMappingTools/pygmt/actions/runs/4318832687/jobs/7537596112), but passes on all other CI runs.
|
Hmm, I don't see why the band coordinate would be The y and x coordinates are of different lengths, but the numbers look the same. Not sure what we can do about that, but let me open up a patch. |
Description of proposed changes
New dataset function to load XYZ tiles! Uses contextily to retrieve the tiles based on a bounding box.
Preview at https://pygmt-dev--2125.org.readthedocs.build/en/2125/api/generated/pygmt.datasets.load_tile_map.html
This is the first part of #2115 that does the tile loading and georeferencing to an
xarray.DataArray
. A follow up PR will be needed to take the outputxarray.DataArray
frompygmt.datasets.load_tile_map
and plot it usingfig.grdimage
(to be discussed in #2115).Usage:
Alternatively, they can use a custom Web Map Tile url like so:
References:
Addresses first part of #2115
Reminders
make format
andmake check
to make sure the code follows the style guide.doc/api/index.rst
.Slash Commands
You can write slash commands (
/command
) in the first line of a comment to performspecific operations. Supported slash commands are:
/format
: automatically format and lint the code/test-gmt-dev
: run full tests on the latest GMT development version