Skip to content
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

Enhanced OGC reader #2191

Merged
merged 1 commit into from Jul 24, 2023
Merged

Enhanced OGC reader #2191

merged 1 commit into from Jul 24, 2023

Conversation

mcuntz
Copy link
Contributor

@mcuntz mcuntz commented Jun 11, 2023

Added support for general EPSG projections in OGC clients WMSRasterSource, WMTSRasterSource, and WFSGeometrySource.

Try to determine geometries from other map servers than the hard-coded mapserver.gis.umn.edu.

Rationale

I tried to use the WFS support with a French server, which did not work because:

  1. the server used the projection EPSG:2154, and
  2. after implementing support for cartopy.crs.epsg, it still did not plot my desired features.

So I

  1. implemented cartopy.crs.epsg as an addition to the known projections. There were only 7 projections known to the OGC readers. Now, all EPSG codes should work.
  2. I try to guess the geometries of a given server because the map server was, strangely, hard-coded to be http://mapserver.gis.umn.edu/mapserver and the geometries were named msGeometry. I search the basic url in the features now and assume that the geometries have geom or Geom in their name. This is not perfect but I do not know any better; this is the first time I use web services for shapes.
  3. While implementing another test case in mpl/test_features.py, I also changed the test case for natural earth from ax.add_feature(cfeature.BORDERS, linestyle=':') to ax.add_feature(cfeature.BORDERS, linestyle='-') because this failed on my machine only due to slightly different dots. Should be more robust know.

Implications

I stayed with the known projections in _URN_TO_CRS and simply added EPSG on top. I had first used the information in cartopy.crs.epsg but the information in EPSG:3031 (Antarctica) was weird and wrong in my opinion.

Guessing the geometries is a layman coding but should be o.k. until it does not work with another server ;-)

@mcuntz
Copy link
Contributor Author

mcuntz commented Jun 11, 2023

Just took back the change ax.add_feature(cfeature.BORDERS, linestyle=':') to ax.add_feature(cfeature.BORDERS, linestyle='-') for the test case of natural earth because tests fail on github otherwise.

@mcuntz
Copy link
Contributor Author

mcuntz commented Jun 11, 2023

flake8 checks fail on

            if ((str(default_urn) not in _URN_TO_CRS) and
                (':EPSG:' not in str(default_urn))):
                raise ValueError(
                    f'Unknown mapping from SRS/CRS_URN {default_urn!r} to '
                    'cartopy projection.')

with
lib/cartopy/io/ogc_clients.py:747:17: E129 visually indented line with same indent as next logical line
If I put in some spaces like

            if ( (str(default_urn) not in _URN_TO_CRS) and
                 (':EPSG:' not in str(default_urn)) ):
                raise ValueError(
                    f'Unknown mapping from SRS/CRS_URN {default_urn!r} to '
                    'cartopy projection.')

flake8 also fails complaining that there is a space after '('.

I don't know how to resolve this :-(

@akrherz
Copy link
Contributor

akrherz commented Jun 11, 2023

Try this

    if (str(default_urn) not in _URN_TO_CRS) and (
        ":EPSG:" not in str(default_urn)
    ):
        raise ValueError(
            f"Unknown mapping from SRS/CRS_URN {default_urn!r} to "
            "cartopy projection."
        )

@@ -666,7 +696,17 @@ def __init__(self, service, features, getfeature_extra_kwargs=None):
raise ImportError(_OWSLIB_REQUIRED)

if isinstance(service, str):
# base url without http:// or https://, and :port
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit unclear exactly what you're doing here, but can you use urllib's parsing to make this more explicit?
https://docs.python.org/3/library/urllib.parse.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I use urlparse now and added comments.

@pytest.mark.mpl_image_compare(filename='wfs_france.png')
def test_wfs_france():
ax = plt.axes(projection=ccrs.epsg(2154))
url = 'https://agroenvgeo.data.inra.fr:443/geoserver/wfs'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if it would be beneficial for us to start mocking servers rather than relying on external sources? Basically, if it'd be reasonable to add both the XML you download from there and the image to the repository and then make a Mock response from that url. Then we wouldn't need these network marks anymore.

This doesn't have to be undertaken here, it is more me just wondering how reliable these external servers are and if there is a way around hitting them constantly if they don't like that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use vcrpy to record and playback http traffic. That would be a low-cost way to "mock" it without questioning whether there's value in having the mock. It gets messy since the only true measure of "works" is "does it communicate with this resources that's outside our control".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pytest plugin for VCR looks like an easy solution. However, I leave this to the main developers because it adds new dependencies to cartopy testing.

lib/cartopy/io/ogc_clients.py Outdated Show resolved Hide resolved
lib/cartopy/io/ogc_clients.py Outdated Show resolved Hide resolved
Comment on lines +596 to +600
with warnings.catch_warnings():
warnings.simplefilter("ignore")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the warning we are ignoring? We should at least mention why this is here with a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is a UserWarning from .to_dict() from proj because the dict does not give all details of each projection. But we only check for the units here.
I added a comment.

Comment on lines 872 to 873
# Try to get other geometries from other servers than
# http://mapserver.gis.umn.edu/mapserver
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to add an extra line or two above to make that loop work for your case? i.e. can you add something above to change the tree.findall() call like updating the _MAP_SERVER_NS or just adding an extra block to find all of your geom tags?

This just reads as a lot of copied code, so another option would be to make a function that both loops could use too. Something like this to update your geometry lists that could be re-used in both loops.

`linear_rings_data, linestrings_data, point_data = _get_geometries(node, linear_rings_data, linestrings_data, point_data)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

O.K. I was trying to not change existing code. Now it is included in the same loop.

lib/cartopy/io/ogc_clients.py Outdated Show resolved Hide resolved
Added support for general EPSG projections in OGC clients WMSRasterSource, WMTSRasterSource, and WFSGeometrySource.

Try to determine geometries from other map servers than the hard-coded mapserver.gis.umn.edu.
Copy link
Contributor

@greglucas greglucas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks, @mcuntz!

@greglucas greglucas merged commit 3af094f into SciTools:main Jul 24, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants