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

Updated Ordnance Survey img_tile to new OS API #2105

Merged
merged 9 commits into from
Nov 30, 2022
Merged

Conversation

dchirst
Copy link
Contributor

@dchirst dchirst commented Nov 23, 2022

Rationale

The Ordnance Survey API has been updated and refactored, breaking the original API that Cartopy used (#2036). Cartopy needs to be brought up to date with OS' newest APIs.

Implications

The Ordnance Survey img tile should now work!

The new api does not support the Night layer, and only supports Leisure for EPSG:27700. Therefore, these 2 map styles will no longer work with the new API in Cartopy (I only implemented support for EPSG:3857).

lib/cartopy/io/img_tiles.py Outdated Show resolved Hide resolved
lib/cartopy/io/img_tiles.py Outdated Show resolved Hide resolved
lib/cartopy/tests/test_img_tiles.py Outdated Show resolved Hide resolved
lib/cartopy/tests/test_img_tiles.py Outdated Show resolved Hide resolved
lib/cartopy/tests/test_img_tiles.py Outdated Show resolved Hide resolved
lib/cartopy/tests/test_img_tiles.py Outdated Show resolved Hide resolved
@dchirst
Copy link
Contributor Author

dchirst commented Nov 23, 2022

CLA signed

@SciTools-assistant SciTools-assistant removed the Blocked: CLA needed See https://scitools.org.uk. Submit the form at: https://scitools.org.uk/cla/v4/form label Nov 23, 2022
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.

Overall this looks pretty good I think, I don't have an API key to test out locally and not sure if we have one on the remote CI... I put a few comments for things to consider, but those could always be follow-up PRs to extend the capability too if that is of interest.

The docstring of the class should be updated to inform users of what the new specifications and links are. Some of the current links are broken.
https://osdatahub.os.uk/docs/wmts/technicalSpecification

lib/cartopy/io/img_tiles.py Show resolved Hide resolved
f'tilematrixSet=EPSG%3A3857&style=true&layer={self.layer}%203857&'
f'SERVICE=WMTS&REQUEST=GetTile&format=image%2Fpng&'
f'TileMatrix=EPSG%3A3857%3A{z}&TileRow={y}&TileCol={x}')
return f"https://api.os.uk/maps/raster/v1/zxy/" \
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like they do still have the WMTS endpoint with query parameters if you want to use that still?
https://api.os.uk/maps/raster/v1/wmts?request=getcapabilities&key=
from: https://osdatahub.os.uk/docs/wmts/technicalSpecification

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 the WMTS endpoint is still available. I thought the zxy endpoint was a bit more concise, so I went with that instead. Do you think it'd be better to keep it as a WMTS endpoint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@greglucas just following up on this - would a WMTS be better than a ZXY endpoint?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, I don't think we need to at this point. If you or someone else wants more flexibility in the future that can be a follow-up PR.

@greglucas greglucas merged commit 397cbe0 into SciTools:main Nov 30, 2022
@greglucas
Copy link
Contributor

Thanks @dchirst!

@QuLogic QuLogic added this to the 0.22 milestone Nov 30, 2022
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.

5 participants