Migrate ign wmts services #166

salahelfarissi · 2024-03-22T15:01:43Z

#165
This PR migrates the ign WMTS services to the new service: https://data.geopf.fr/wmts?SERVICE=WMTS&VERSION=1.0.0&REQUEST=GetCapabilities

The new services don't have a categorizing key, like agriculture...
So I removed the apiKey as well as its test.

Edit
The test_callable is actually needed.

martinfleis

Thanks!

@HaudinFlorence could you do a review of this change please?

martinfleis · 2024-03-22T15:12:49Z

xyzservices/tests/test_lib.py

-def test_callable():
-    # only testing the callable functionality to override a keyword, as we
-    # cannot test the actual providers that need an API key
-    original_key = str(xyz.GeoportailFrance.plan["apikey"])
-    updated_provider = xyz.GeoportailFrance.plan(apikey="mykey")
-    assert isinstance(updated_provider, TileProvider)
-    assert "url" in updated_provider
-    assert updated_provider["apikey"] == "mykey"
-    # check that original provider dict is not modified
-    assert xyz.GeoportailFrance.plan["apikey"] == original_key


This does not necessarily test xyz.GeoportailFrance tiles but the callable functionality of TileProvider object. We need to keep the test, just use different TileProvider in there.

OK.
I will.

All tests passing.

HaudinFlorence · 2024-03-25T10:30:54Z

@martinfleis @salahelfarissi Hello. I will have a look within this week, I started today but it will take me some time to be completely back on tracks on the topics, after almost 2 years. Hoping there is not too much time constraint for the PR to be merged right away.

martinfleis · 2024-03-25T10:35:39Z

@HaudinFlorence thanks! There's no rush, it seems that the old URLs are still working. At least according to our CI.

HaudinFlorence · 2024-03-29T08:45:30Z

@HaudinFlorence thanks! There's no rush, it seems that the old URLs are still working. At least according to our CI.

@martinfleis Thanks for your reply. It is still on my to-do list and will try to have a look in the following days.

HaudinFlorence · 2024-04-02T17:16:58Z

Hello. I have tested the PR and took some time to check that the script _compress_providers.py works properly for the GeoportailFrance data. It seems ok. I am still not very used to usepytest yet so I haven't really checked the tests.

Here are my main comments/remarks (some of them are maybe not directly related to the migration itself but more to some maintenance of GeoportailFrance tilelayers providers:

I am wondering if it would not be a good idea to add a test for a GeoportalFrance tilelayer.
It is a detail, but the way the name was built from the variant (my proposition initially) leads that the last name never has a capital letter. Maybe this could be a good idea to change this for consistency
I have had a look at the gallery to check if the basemaps were rendered with the new provider generated. It seems that some basemaps referred to as broken are not (at least there is something on the map). On the contrary on other maps, there is nothing appearing but I haven't checked if it is due to some broken status or to a focus on a area where there is no data to display.
Here is the list (that may not be exhaustive) of basemaps that are not broken anymore:
GeoportailFrance.Ocsge_Couverture_2002
GeoportailFrance.Ocsge_Couverture_2011
GeoportailFrance.Ocsge_Couverture_2014
GeoportailFrance.Ocsge_Couverture_2016
GeoportailFrance.Ocsge_Usage_2002
GeoportailFrance.Ocsge_Usage_2011
GeoportailFrance.Ocsge_Usage_2014
GeoportailFrance.Ocsge_Usage_2016
GeoportailFrance.Orthoimagery_Orthophotos_Coast2000
GeoportailFrance.Orthoimagery_Orthophotos_Geneve
GeoportailFrance.Points_Injection_Biomethane

HaudinFlorence · 2024-04-02T17:24:52Z

provider_sources/_compress_providers.py

+    # Rename for better readability
+    name_parts = [part.lower().capitalize().replace("-", "_") for part in variant.split(".")]
+    name = "_".join(name_parts)
+


This name building should maybe be reviewed to enable the last word of the name to have a capital letter. Maybe a special case should be used for acronyms whatever position it has in the name.

provider_sources/_compress_providers.py

martinfleis · 2024-04-02T18:11:54Z

I am wondering if it would not be a good idea to add a test for a GeoportalFrance tilelayer.

We have them. All if them are autogenerated. You can see them in the pytest log here: https://github.com/geopandas/xyzservices/actions/runs/8393293189/job/22987943322?pr=166

HaudinFlorence · 2024-04-02T22:08:36Z

I am wondering if it would not be a good idea to add a test for a GeoportalFrance tilelayer.

We have them. All if them are autogenerated. You can see them in the pytest log here: https://github.com/geopandas/xyzservices/actions/runs/8393293189/job/22987943322?pr=166

Ok. Nice. So sorry, you can forget about my comment. As mentionned I did not checked tests in details and am not very familir to python tests. But now I remember that at some point I was running them and we were trying to reduce the number of failing ones when we introduce the GeoportailFrance tile layers.
By the way, having a look back again at the old PR introducing these new tilelayers #126, maybe my comments related to the broken basemaps does not hold considering your comment at that time, that some basemaps were set to broken because their tests were failing even when trying to use relevant tiles.

salahelfarissi · 2024-04-03T06:41:14Z

Should I work on the name build or is it okay for this PR ?

martinfleis · 2024-04-03T07:15:29Z

I would try to ensure the names are exactly as they were. It may not be optimal but it is released and some people may expect the exact names.

HaudinFlorence · 2024-04-03T08:23:51Z

I would try to ensure the names are exactly as they were. It may not be optimal but it is released and some people may expect the exact names.

Yes you are right @martinfleis. Changing the name now may be misleading. So I don't have any other comment. The PR seems ok to me. It seems to properly generates the data for the providers for GeoportailFrance basemaps. Thanks for your work @salahelfarissi

salahelfarissi · 2024-04-03T09:42:59Z

These were the modified names (21 rows).

Now fixed.

martinfleis

Let's give it a go. Thanks!

Migrate ign wmts services

11c3983

salahelfarissi mentioned this pull request Mar 22, 2024

IGN (cx.providers.GeoportailFrance.orthos) migrated their services (wxs.ign.fr to data.geopf.fr) #165

Closed

martinfleis reviewed Mar 22, 2024

View reviewed changes

salahelfarissi added 6 commits March 22, 2024 16:19

Handle broken providers

d71922a

Test callable of TileProvider

83770ea

Add api_key to ign services

b7b0b14

Add possible broken urls

1168fc0

Add missing NLS apikeys

2028f33

Add overmissed apikey

4a28dc0

HaudinFlorence reviewed Apr 2, 2024

View reviewed changes

provider_sources/_compress_providers.py Show resolved Hide resolved

salahelfarissi added 2 commits April 3, 2024 11:18

Ensure consistent naming

f5eca80

Update providers

edcd8c0

martinfleis added 3 commits April 3, 2024 17:02

rm broken labels to see which are actually broken

0af8eef

mark those actually broken as broken

4f9e4f7

compress

16be1d8

martinfleis approved these changes Apr 3, 2024

View reviewed changes

martinfleis merged commit 3f418ca into geopandas:main Apr 3, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate ign wmts services #166

Migrate ign wmts services #166

salahelfarissi commented Mar 22, 2024 •

edited

Loading

martinfleis left a comment

martinfleis Mar 22, 2024

salahelfarissi Mar 22, 2024

salahelfarissi Mar 22, 2024

salahelfarissi Mar 22, 2024

HaudinFlorence commented Mar 25, 2024 •

edited

Loading

martinfleis commented Mar 25, 2024

HaudinFlorence commented Mar 29, 2024

HaudinFlorence commented Apr 2, 2024 •

edited

Loading

HaudinFlorence Apr 2, 2024

martinfleis commented Apr 2, 2024

HaudinFlorence commented Apr 2, 2024 •

edited

Loading

salahelfarissi commented Apr 3, 2024

martinfleis commented Apr 3, 2024

HaudinFlorence commented Apr 3, 2024 •

edited

Loading

salahelfarissi commented Apr 3, 2024

martinfleis left a comment

Migrate ign wmts services #166

Migrate ign wmts services #166

Conversation

salahelfarissi commented Mar 22, 2024 • edited Loading

martinfleis left a comment

Choose a reason for hiding this comment

martinfleis Mar 22, 2024

Choose a reason for hiding this comment

salahelfarissi Mar 22, 2024

Choose a reason for hiding this comment

salahelfarissi Mar 22, 2024

Choose a reason for hiding this comment

salahelfarissi Mar 22, 2024

Choose a reason for hiding this comment

HaudinFlorence commented Mar 25, 2024 • edited Loading

martinfleis commented Mar 25, 2024

HaudinFlorence commented Mar 29, 2024

HaudinFlorence commented Apr 2, 2024 • edited Loading

HaudinFlorence Apr 2, 2024

Choose a reason for hiding this comment

martinfleis commented Apr 2, 2024

HaudinFlorence commented Apr 2, 2024 • edited Loading

salahelfarissi commented Apr 3, 2024

martinfleis commented Apr 3, 2024

HaudinFlorence commented Apr 3, 2024 • edited Loading

salahelfarissi commented Apr 3, 2024

martinfleis left a comment

Choose a reason for hiding this comment

salahelfarissi commented Mar 22, 2024 •

edited

Loading

HaudinFlorence commented Mar 25, 2024 •

edited

Loading

HaudinFlorence commented Apr 2, 2024 •

edited

Loading

HaudinFlorence commented Apr 2, 2024 •

edited

Loading

HaudinFlorence commented Apr 3, 2024 •

edited

Loading