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

Add a cartopy.io.img_tiles.Stamen class #1188

Merged
merged 7 commits into from
Nov 16, 2018
Merged

Conversation

pelson
Copy link
Member

@pelson pelson commented Nov 15, 2018

Rationale

Replaces #1013 to generalise the Stamen class of image tiles, thus giving access to all existing and future Stamen styles (following the same URL scheme).
Also adds a more abstract GoogleWTS tiler class to allow more appropriate subclassing.

Implications

lib/cartopy/io/img_tiles.py Outdated Show resolved Hide resolved
lib/cartopy/io/img_tiles.py Outdated Show resolved Hide resolved
lib/cartopy/io/img_tiles.py Show resolved Hide resolved
lib/cartopy/io/img_tiles.py Show resolved Hide resolved
@pelson
Copy link
Member Author

pelson commented Nov 15, 2018

@QuLogic - updated as per review. Thanks for the feedback.

@QuLogic
Copy link
Member

QuLogic commented Nov 15, 2018

Looking at the warnings in the doc build, it seems like there might be a few other examples that use the StamenTerrain class.

@manugarri
Copy link
Contributor

Oh nice!


import cartopy.crs as ccrs


class GoogleTiles(object):
class GoogleWTS(six.with_metaclass(ABCMeta, object)):
Copy link
Member

Choose a reason for hiding this comment

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

I love that this is getting the WTMS in the name but is it still worth keeping "Google" in there? Why not taking this opportunity to make the class name more generic? I believe GoogleWTS may throw users, who don't read the docs, off by thinking it only works for Google Tiles.

Copy link
Member Author

Choose a reason for hiding this comment

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

A fine observation. I agree entirely with the sentiment, but it is really hard to find a name that doesn't include the word Google - this isn't a general WMTS implementation AFAICT - it really does build upon Google mercator and uses (as far as I can tell) a non-standard y indexing scheme.

I suspect there are further abstractions that would be useful (there is a 'Microsoft' Quadtree implementation in here that really only needs a small subset of the GoogleWTS class's behaviour).

In short - I'm 100% in favour of finding a name that represents the scheme that Google implemented (I think it was them at least), but at present don't have a better suggestion.

@pelson
Copy link
Member Author

pelson commented Nov 16, 2018

@QuLogic - merge at will. GoogleWTS rename is a thing I'd happily support in a subsequent PR / at a later release.

@QuLogic QuLogic merged commit 4d81618 into SciTools:master Nov 16, 2018
@QuLogic QuLogic added this to the 0.17 milestone Nov 16, 2018
@pelson pelson deleted the stamen_class branch December 3, 2018 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants