Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
load_tile_map: Add the new parameter 'crs' to set the CRS of the returned dataarray #3554
load_tile_map: Add the new parameter 'crs' to set the CRS of the returned dataarray #3554
Changes from 6 commits
3304104
cfb69d2
00e2605
3265240
7660e2c
421c167
8346af7
4ed799c
6733e19
c1d55b0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Maybe put the
rasterio
imports in the rioxarray block below instead of here.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.
rasterio
is a known dependency ofcontextily
(xref: https://github.com/geopandas/contextily/blob/main/pyproject.toml), so it makes more sense to assume that they're installed together.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.
Rasterio is also a dependency of
rioxarray
😆. I would argue thatrasterio
is more tightly coupled torioxarray
, and sincerasterio
/rioxarray
are both used for their projection system capabilities, it would make more sense to put those together. But ok too if you want to keep it here.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.
At https://contextily.readthedocs.io/en/latest/reference.html#contextily.bounds2img, there is this sentence:
While tilemaps are usually served in EPSG:3857, I know that there are some tilemaps that can be served in a different projection system by default. I'm hesitant to use this default of EPSG:3857 here, unless we can be confident that contextily only supports EPSG:3857 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.
Looking at the contexily source codes, I think contexitly always assumes that CRS is EPSG:3857, at least when writing the image to raster files
https://github.com/geopandas/contextily/blob/eafeb3c993ab43356b65e7a29b9dd6df3ac76f00/contextily/tile.py#L358
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.
That is for the
warp_tiles
function. I don't see anything about EPSG:3857 in thebounds2img
function (though it's calling several functions and I haven't checked too closely).Non-Web Mercator (EPSG:3857) XYZ tiles are not common, but they do exist, e.g. Openlayers allows configuring the projection system of the returned 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.
I meant this line:
https://github.com/geopandas/contextily/blob/eafeb3c993ab43356b65e7a29b9dd6df3ac76f00/contextily/tile.py#L175
In bounds2raster, it first calls
bounds2img
to get theimage
andextent
(can be any CRS), but when writing to a raster image,bounds2raster
assumes theimage
isEPSG:3857
.Looking at the xyzservices (https://github.com/geopandas/xyzservices/blob/c847f312d2e67a0a45ceae119fdcdf7cf60858b6/provider_sources/xyzservices-providers.json#L50), I can also see some providers that has the
crs
attribute, but it's unclear how thecrs
attribute is handled in contexitly.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 tried using
xyzservices.providers.MapTiler.Basic4326
which comes in EPSG:4326:produces
The extent doesn't seem to be correct (it should be plotting from Greenwich to Asia), possibly a bug in
contextily
hardcoding to EPSG:3857. But it does show that non-Web Mercator XYZ tiles are indeed 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.
Found geopandas/contextily#119, and had a closer look at the
bounds2img
code, specifically these lines - https://github.com/geopandas/contextily/blob/00ca0318033e69e29a164ce571d9c932a057ecc6/contextily/tile.py#L270-L272. It seems likecontextily
is usingmercantile
which only supports Spherical Mercator (EPSG:3857), see mapbox/mercantile#109 (comment). Unlesscontextily
switches to something like https://github.com/developmentseed/morecantile in the future, then it would only 'work' with 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.
https://github.com/geopandas/contextily/blob/eafeb3c993ab43356b65e7a29b9dd6df3ac76f00/contextily/tile.py#L256
When
ll=True
,bounds2img
calls the private_sm2ll
function to convert bounds in Spherical Mercator coordinates to lon/lat. I believe "EPSG:3857" is hardcoded/assumed in many places in the contextily package.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.
How about checking
source.crs
(an attribute from an instance of thexyzservices.TileProvider
class), and if it is present, use that as the default crs. If not, fallback to 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.
Sounds good. Done in 6733e19.
Please note that this line of code works for
TileProvide
,str
andNone
.str
andNone
don't havecrs
, soEPSG:3857
is returned.Edit: renamed
_default_crs
to_source_crs
in c1d55b0.Please note that, currently, the returned xarray.DataArray is still "EPSG:3857" even if the source CRS is not (i.e. we don't the reprojection). This may not be ideal.
Check warning on line 174 in pygmt/datasets/tile_map.py
pygmt/datasets/tile_map.py#L173-L174
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
Figure.tilemap
method no longer requiresrioxarray
explicitly. Instead,load_tile_map
will raise the error.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.
If
lonlat=True
, set the CRS to letload_tile_map
do the raster reprojection for us.