-
Notifications
You must be signed in to change notification settings - Fork 363
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
ENH: Allow Natural Earth version to be specified #2351
base: main
Are you sure you want to change the base?
Conversation
Do we also need something to expose the version option on |
Yes, thanks. Version should definitely be an option there too but I wasn't tracking that feature - have added it in now. |
Related, I have linked there to https://github.com/nvkelso/natural-earth-vector/releases as a good source for version information, because it was confusing that most of the versions shown on https://www.naturalearthdata.com/ (e.g. 4.0.0, 2.0.0, etc.) are not available on the AWS service to download, yet there are many newer versioned releases that are available which are not shown on https://www.naturalearthdata.com/. |
b5d5a72
to
1654f0a
Compare
CI helped pick up an omission of version in one place in NaturalEarthFeature - a clean version is now pushed with that edit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An elegant solution, thank you for implementing this!
I have not yet tested it myself, but it looks like a viable solution to me
@@ -37,7 +37,7 @@ def test_natural_earth(): | |||
@pytest.mark.mpl_image_compare(filename='natural_earth_custom.png') | |||
def test_natural_earth_custom(): | |||
ax = plt.axes(projection=ccrs.PlateCarree()) | |||
feature = cfeature.NaturalEarthFeature('physical', 'coastline', '50m', | |||
feature = cfeature.NaturalEarthFeature('physical', 'coastline', '50m', '5.1.0', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you find that version 5.1.0
successfully passed this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Yes, no diff between the baseline image here of Iceland and when using 5.1.0
@@ -83,7 +83,8 @@ class TestRivers: | |||
def setup_class(self): | |||
RIVERS_PATH = shp.natural_earth(resolution='110m', | |||
category='physical', | |||
name='rivers_lake_centerlines') | |||
name='rivers_lake_centerlines', | |||
version='5.0.0') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, did this specific version (5.0.0
) of rivers pass the image comparison tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it was passing. Note there is no image comparison here, the test just looks at a few very specific features and there was no change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't test this out yet, but would you be able to add some description to the docstrings about where the versions are cached/stored? I think they are stored separately looking at the code, but just want to make sure we aren't getting a mixture of versions in the default namespace. If the latest is 5.1.2
and I explicitly ask for 5.1.2
do we have to download/store both?
_NE_URL_TEMPLATE = ('https://naturalearth.s3.amazonaws.com/' | ||
'{version}/{resolution}_{category}/' | ||
'ne_{resolution}_{name}.zip') | ||
|
||
default_spec = ('shapefiles', 'natural_earth', '{category}', '{version}', | ||
'ne_{resolution}_{name}.shp') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you be able to inject this into the current NE template? It looks like the only difference is {version}/
, and I'm wondering if you could put that in and always have a version with something like _version_string = "" if version is None else f"{version}/"
.
ping @lgolston, I had a few questions on this, but it looks like it may be very close to ready to go? |
Rationale
As requested in #2293, it would be useful for reproducibility to be able to specify what Natural Earth version to use, in addition to current default behavior of downloading the latest version.
Implications
This enhancement is added. If a version is specified, it will be downloaded to subfolders within cultural or physical corresponding to the version requested and then used.
Checklist
Testing: Since Natural Earth is already in several places in the test suite, I modified one of them to now use a specific version.