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

ENH: Allow Natural Earth version to be specified #2351

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lgolston
Copy link
Contributor

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.

@rcomer
Copy link
Member

rcomer commented Mar 15, 2024

Do we also need something to expose the version option on NaturalEarthFeature, similar to how scale is handled?

@lgolston
Copy link
Contributor Author

Do we also need something to expose the version option on NaturalEarthFeature, similar to how scale is handled?

Yes, thanks. Version should definitely be an option there too but I wasn't tracking that feature - have added it in now.

@lgolston
Copy link
Contributor Author

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/.

@lgolston
Copy link
Contributor Author

CI helped pick up an omission of version in one place in NaturalEarthFeature - a clean version is now pushed with that edit.

@rcomer rcomer requested a review from kaedonkers March 17, 2024 09:16
@rcomer rcomer linked an issue Mar 17, 2024 that may be closed by this pull request
Copy link
Member

@kaedonkers kaedonkers left a 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',
Copy link
Member

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?

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! 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')
Copy link
Member

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?

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, 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.

@rcomer rcomer self-requested a review April 26, 2024 16:22
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.

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?

Comment on lines +410 to +415
_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')
Copy link
Contributor

@greglucas greglucas Jun 8, 2024

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}/".

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.

Natural Earth: Include version in URL
4 participants