Coastlines update #1823
Coastlines update #1823
Conversation
|
Undid 00f95f1 and instead used black to achieve correct code format. I do not know why the Mac OS test fails though. |
There was a problem hiding this comment.
I wonder if it is possible to get the latest version?
https://www.ngdc.noaa.gov/mgg/shorelines/data/gshhg/latest/
That would be complicated by the version number being updated in the URL, but it would be nice to just always get the latest updates when possible.
Can you squash all of your commits? It is a bit complicated to follow when adding/removing content. You can safely ignore the Python3.6 failing test right now, it isn't related to your PR.
|
The changing URL is indeed the reason for me not choosing the latest version. I agree this would be nice, but I cannot think of a safe way to implement this. I could try something like I implemented your suggestion about the |
|
👍 That may be overcomplicating things to parse the latest page to grab the url. I wonder if adding a version number to the arguments of the downloader would be useful though, so we could just update the version number when it is bumped? Then you could try both the latest location and old location to download that version number which would gracefully handle the move from latest to old. Something like: try:
self._urlopen(latest_url)
except HTTPError:
self._urlopen(oldversions_url)I think what you have is a good update though, so don't feel obligated to add the suggestion, it can be handled in a follow-up PR as well. |
|
Okay I will try this, let's do this in this PR. |
lib/cartopy/io/shapereader.py
Outdated
| try: | ||
| shapefile_online = self._urlopen(url) | ||
| except HTTPError: | ||
| warnings.warn( |
There was a problem hiding this comment.
Does this mean that if ngdc updates to a new version, users are going to get a warning that they can't fix (without resorting to a warning filter)? I'm not a fan of issuing warnings when users are doing nothing wrong and can't fix it.
There was a problem hiding this comment.
Yes this is what it means, but ...
- it happens only once (and a Download warning also happens when the coastlines are downloaded anyway)
- it is better than having no warning but the bugs of an old coastline data set.
Alternative would be to either download the older version in the first place as before or leave the warning away.
There was a problem hiding this comment.
There is a third option, which is what I was trying to hint at, which is to put a version number in somewhere.
_GSHHS_URL_TEMPLATE = f'https://www.ngdc.noaa.gov/mgg/shorelines/data/gshhg/latest/gshhg-shp-{version}.zip'Then on the except you would do:
url = f'https://www.ngdc.noaa.gov/mgg/shorelines/data/gshhs/oldversions/version{version}/gshhg-shp-{version}.zip'I assume that the 2.3.7 will be moved over to the archive at some point when a new one comes up, and this would catch that case assuming they keep their naming convention.
There was a problem hiding this comment.
Shouldn't we have something in the except that works even if the naming convention is changed?
There was a problem hiding this comment.
👍 Add a fallback if both of those fail to the 2.3.6 version if both throw the URL not found error?
There was a problem hiding this comment.
Okay, here is my attempt
|
Okay, did squash everything into one again. Maybe someone can check. |
| zfh = ZipFile(io.BytesIO(shapefile_online.read()), 'r') | ||
| shapefile_online.close() | ||
|
|
||
| # Iterate through all scales and levels and extract relevant files. | ||
| modified_format_dict = dict(format_dict) | ||
| scales = ('c', 'l', 'i', 'h', 'f') | ||
| levels = (1, 2, 3, 4) | ||
| levels = (1, 2, 3, 4, 5, 6) |
There was a problem hiding this comment.
Can you update the documentation to indicate it goes up to 6 now?
cartopy/lib/cartopy/feature/__init__.py
Lines 327 to 328 in 4ca93c1
There was a problem hiding this comment.
did that, will squash once more
|
sorry this review request was a mistake of mine |
|
Looks like I did something wrong with the squash. |
|
Keep going with this PR if you can. You're close on the rebase I think! Try this: git fetch upstream
git rebase -i upstream/masterThen delete the extra commit from your work and keep this one. |
error handling and doc update.
|
Okay, thanks. Let's see. |
There was a problem hiding this comment.
@dopplershift, I think the warning you were concerned with is removed now. I'll leave open for a bit in case you have any comments on this.
Yes I removed it. |
|
Thanks for updating this, @MHBalsmeier! |
Rationale
The GSHHS coastlines that cartopy was automatically downloading were quite old, lots of newer versions came out. Several issues were raised about this (#1353, #1659). I updated the coastlines from 2.2.0 to 2.3.6.
Implications
More up-to-date and better coastlines are used that include bugfixes and resolve some shape warnings that were frequently thrown.