Remove Stamen #1811

ocefpaf · 2023-10-04T16:18:28Z

ocefpaf · 2023-10-04T16:34:48Z

docs/index.rst

@@ -22,7 +22,7 @@ It enables both the binding of data to a map for choropleth visualizations
 as well as passing rich vector/raster/HTML visualizations as markers on the map.

 The library has a number of built-in tilesets from OpenStreetMap,
-Mapbox, and Stamen, and supports custom tilesets.
+Mapbox, etc, and supports custom tilesets.


@Conengmo I wonder if we should leave OSM as default and remove everything else in lieu of xyzservices.

Or we could make xyzservices a required dependency (pure python, no dependencies) and offload all of it, while giving users all available tiles by default.

That is kind of what I was thinking. Making it a mandatory dependency and using it everywhere instead of having the templates for CartoDB, which was the last one to survive the API key revolution ;-p

Then we agree :). If we want to do that, we should expose xyzservices via the query_name method as we do in geopandas or contextily, so you don't need to interact with xyzservices directly.

What do you think @Conengmo ? Should I start that in this PR?

Really interesting thought! Can you give me a moment? I don’t want to rush my thoughts here. I’ll get back this weekend if that’s okay.

We are not in a hurry 😄
But b/c this is kind of a breaking change I would like to hold the next release to ship this with it, if we decide to implement it.

PS: the devil is in the details, we have tons of examples and docs that will request some re-writing. However, the disruption on end user won't be too big, I hope.

we have tons of examples and docs that will request some re-writing. However, the disruption on end user won't be too big, I hope.

Why? Apart from Stamen maps not being available, which is nothing we can influence, it should be completely backwards compatible. Can you point me to examples that would need re-writing?

I'm on mobile now to check but my guess are the names we use in the tile keywords in the do docstring and examples. Those would change to match XYZ, no? I'd love for us to use the same as geopandas to reduce the cognitive load on users

If exposed via query_name they should, with the exception of "OpenStreetMap", which needs to be mapped to "OpenStreetMap.Mapnik" but we can sort this one case internally I suppose.

Conengmo

Good initiative this! We have to remove these indeed.

I agree with making xyzservices a requirement and removing the tile templating from Folium. It's a good idea. xyzservices is light-weight and uses leaflet-providers directly. That's great for Folium.

I'd like to make it a non-breaking change. So Map(tiles="openstreetmap") and Map(tiles="cartodbpositron") are still supported. I like the idea of using Bunch.query_name() for this. And then mapping "openstreetmap" to "openstreetmap.mapnik" internally for backwards compatibility.

In that way the API for Folium won't change from a users perspective. They can still provide a tileset name (which will then use xyzservices internally), or a url + attribution, or a xyzservices TileProvider object.

I wouldn't do the xyzservices adoption in this PR. Removing Stamen is something we have to do anyway, whether we adopt xyzservices or not.

I would suggest to include this change in a 0.15.0 release we do after this one is merged. So there are no issues with the Stamen deprecation before the end of October. Also there's some stuff on main waiting to be released, so I'd rather not wait for more changes before we release. Note that we also need a 0.7.0 release for Branca in tandem with 0.15.0 for Folium.

Then we can do the xyzservices adoption. Add some tests and make sure our use cases are covered.

Who should do what? How are you in time @ocefpaf? I have some time next week. I'm also planning on using Closember again to go through our open issues once more.

docs/getting_started.md

docs/advanced_guide/custom_panes.md

tests/plugins/test_minimap.py

docs/advanced_guide/custom_panes.md

martinfleis · 2023-10-10T10:04:31Z

Sounds good to me. Happy to be of any assistance with the xyzservices implementation if @ocefpaf wants to take a first look at it. You can check how we expose it in contextily or geopandas for inspiration.

Conengmo

I've addressed my own comments. If the tests pass I'll go ahead and merge this. Then push release notes so we can hopefully get a release out before the end of the month.

Conengmo · 2023-10-26T15:44:49Z

@ocefpaf I updated the changelogs. Could you make a Branca 0.7.0 and a Folium 0.15.0 release please?

ocefpaf · 2023-11-06T17:08:07Z

Apologies for the late response but my past few weeks have been complicated.

I'd like to make it a non-breaking change. So Map(tiles="openstreetmap") and Map(tiles="cartodbpositron") are still supported. I like the idea of using Bunch.query_name() for this. And then mapping "openstreetmap" to "openstreetmap.mapnik" internally for backwards compatibility.

Agreed.

I wouldn't do the xyzservices adoption in this PR. Removing Stamen is something we have to do anyway, whether we adopt xyzservices or not.

Done ;-p

I would suggest to include this change in a 0.15.0 release we do after this one is merged. So there are no issues with the Stamen deprecation before the end of October. Also there's some stuff on main waiting to be released, so I'd rather not wait for more changes before we release. Note that we also need a 0.7.0 release for Branca in tandem with 0.15.0 for Folium.

Should I start the engines from branca 0.7.0 and folium 0.15.0? I can do both today.

Then we can do the xyzservices adoption. Add some tests and make sure our use cases are covered.

Who should do what? How are you in time @ocefpaf? I have some time next week. I'm also planning on using Closember again to go through our open issues once more.

I cannot promise anything b/c I'll be moving into a new place in November. If you are willing to do it please go ahead and don't wait for me. If you don't get to this, I will have some time reserved for folium in december.

ocefpaf · 2023-11-06T17:08:29Z

@ocefpaf I updated the changelogs. Could you make a Branca 0.7.0 and a Folium 0.15.0 release please?

On it... I missed this comment while answering above.

martinfleis · 2023-11-06T17:31:07Z

I can do the xyzservices implementation

Conengmo · 2023-11-06T17:45:28Z

Thanks for making the releases @ocefpaf! I hope you are okay, and good luck with whatever's going on 🍀

I'll update the changelog to include the merges of today, and edit the release to include our changelog.

ocefpaf · 2023-11-06T17:54:06Z

I'll update the changelog to include the merges of today, and edit the release to include our changelog.

Apologies, should've done that. I thought we dropped the changelog here in lieu for the auto-generated changelog in the release notes. (I did that in some projects and things are blurry to me now. Sorry!)

Conengmo · 2023-11-06T17:56:15Z

no worries at all :)

remove Stamen

ba9c826

ocefpaf commented Oct 4, 2023

View reviewed changes

ocefpaf marked this pull request as ready for review October 4, 2023 16:35

Conengmo requested changes Oct 10, 2023

View reviewed changes

docs/getting_started.md Outdated Show resolved Hide resolved

docs/advanced_guide/custom_panes.md Outdated Show resolved Hide resolved

tests/plugins/test_minimap.py Outdated Show resolved Hide resolved

docs/advanced_guide/custom_panes.md Outdated Show resolved Hide resolved

Conengmo added 2 commits October 26, 2023 14:48

address comments on docs

7bccc20

Update test_minimap.py

4b74951

Conengmo approved these changes Oct 26, 2023

View reviewed changes

Conengmo merged commit a838646 into python-visualization:main Oct 26, 2023
12 checks passed

ocefpaf deleted the remove_stamen branch November 6, 2023 17:05

RobbeW mentioned this pull request Nov 15, 2023

Folium Tiles Water Color / Deprecated google-deepmind/ithaca#6

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Stamen #1811

Remove Stamen #1811

ocefpaf commented Oct 4, 2023

ocefpaf Oct 4, 2023

martinfleis Oct 4, 2023

ocefpaf Oct 4, 2023 •

edited

Loading

martinfleis Oct 5, 2023

ocefpaf Oct 5, 2023

Conengmo Oct 5, 2023

ocefpaf Oct 5, 2023

martinfleis Oct 5, 2023

ocefpaf Oct 5, 2023

martinfleis Oct 5, 2023

Conengmo left a comment

martinfleis commented Oct 10, 2023

Conengmo left a comment

Conengmo commented Oct 26, 2023 •

edited

Loading

ocefpaf commented Nov 6, 2023

ocefpaf commented Nov 6, 2023

martinfleis commented Nov 6, 2023

Conengmo commented Nov 6, 2023

ocefpaf commented Nov 6, 2023

Conengmo commented Nov 6, 2023

Remove Stamen #1811

Remove Stamen #1811

Conversation

ocefpaf commented Oct 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ocefpaf Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Conengmo left a comment

Choose a reason for hiding this comment

martinfleis commented Oct 10, 2023

Conengmo left a comment

Choose a reason for hiding this comment

Conengmo commented Oct 26, 2023 • edited Loading

ocefpaf commented Nov 6, 2023

ocefpaf commented Nov 6, 2023

martinfleis commented Nov 6, 2023

Conengmo commented Nov 6, 2023

ocefpaf commented Nov 6, 2023

Conengmo commented Nov 6, 2023

ocefpaf Oct 4, 2023 •

edited

Loading

Conengmo commented Oct 26, 2023 •

edited

Loading