geopandas / contextily

Context geo-tiles in Python
https://contextily.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
493 stars 81 forks source link

_calculate_zoom returns negative values if coordinates in epsg:3857 are passed #193

Open JacobThompsonFisher opened 2 years ago

JacobThompsonFisher commented 2 years ago

This is an odd behavior that may not matter, and can possibly be closed immediately, but seemed worth documenting, as a user who just spent an hour debugging something.

If a user queries contextily.tile._calculate_zoom(w, s, e, n) using w,s,e,n from a 3857 shape, the function will return a negative value, presumably because it expects lon/lat inputs.

I would completely ignore this except that the core add_basemap function almost explicitly expects 3857.

If w,s,e,n points in epsg:4326 are passed to the _calculate_zoom function, it returns the expected values within nominal ranges for zoom.

Short test case, also attached as py in .txt `import contextily as ctx

total bounds box of a shape in epsg:3857

box3857 = [-13637706.360897053, 4527514.298503321, -13631479.895919155, 4532126.494783246]

total bounds box of a shape in epsg:4326

box4326 = [-122.503777375, 37.63160800000001, -122.459490625, 37.664412999999996]

calc_zoom on the 3857 bounds

zoom3857 = ctx.tile._calculate_zoom(box3857[0], box3857[1], box3857[2], box3857[3])

calc_zoom on the 4326 bounds

zoom4326 = ctx.tile._calculate_zoom(box4326[0], box4326[1], box4326[2], box4326[3])

print(f'EPSG:3857 result: {zoom3857} \n EPSG:4326 result: {zoom4326}') Contextily_Test_Case.txt `

darribas commented 2 years ago

Hello @JacobThompsonFisher,

Thanks very much for taking the interest in working with contextily! You're right, _calculate_zoom is probably not the most intuitive method in that it expects coordinates in lon/lat even though the zoom is really calculated on a Web Mercator context. The behaviour is documented however:

https://github.com/geopandas/contextily/blob/72c85a1097dfad6f03d2b0b17cd92dfd8171b0ee/contextily/tile.py#L503-L523

And we signal the method is not necessarily intended for regular users by starting the function with the underscore (_).

In terms of going forward, perhaps one user improvement would be to come up with a user-facing method (calculate_zoom) that incorporated CRS switching functionality, so the default could be a bbox in every CRS, and that CRS. This is a bit how CRS management works in add_basemap, which is user-facing. What do you think?

JacobThompsonFisher commented 2 years ago

I could not find that documentation to save my life, definitely a failure to rtfm.

For a user facing function I think you're absolutely right. I am not certain how many users actually need the function, but something which mirrors the bbox+CRS approach in add_basemap would be ideal. That said, probably a low priority given how rarely zoom customization on dynamic values seems to come up in the user base.

martinfleis commented 2 years ago

We have actually discussed this already before in #172. I think we should create a public function that is more user friendly and flexible on input.