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

Allow GSHHS levels 5 and 6 for Antarctica. #2317

Merged
merged 1 commit into from
Jan 25, 2024
Merged

Conversation

kdungs
Copy link
Contributor

@kdungs kdungs commented Jan 24, 2024

Rationale

The documentation for GSHHSFeature already states that levels 1 through 6 are allowed but the code so far only allowed 1 through 4.

As per https://www.ngdc.noaa.gov/mgg/shorelines/data/gshhg/readme.txt levels 5 and 6 were introduced in GSHHS version 2.3.0 in February 2014 to distinguish the Antarctic ice-front (L5) and grounding lines (L6).

Importantly, this also means that L1 does not contain any coastlines for Antarctica, so allowing only L1-L4 is insufficient.

See also https://www.soest.hawaii.edu/pwessel/gshhg/.

Implications

  • Antarctica coastlines are supported.
  • Users have to manually choose L5 or L6 or both.

Side note: there are no existing tests I could piggyback on but "it works on my machine"... Let me know if you have a more structured way to make sure this works for everyone.

The documentation for GSHHSFeature already states that levels 1 through
6 are allowed but the code so far only allowed 1 through 4.

As per https://www.ngdc.noaa.gov/mgg/shorelines/data/gshhg/readme.txt
levels 5 and 6 were introduced in GSHHS version 2.3.0 in February 2014
to distinguish the Antarctic ice-front (L5) and grounding lines (L6).

Importantly, this also means that L1 does not contain any coastlines for
Antarctica, so allowing only L1-L4 is insufficient.

See also https://www.soest.hawaii.edu/pwessel/gshhg/.
@greglucas
Copy link
Contributor

Is that unknown levels error even valuable? Maybe we should just remove that check altogether. What happens if they add a 7, 8 in the future, we would then raise again which doesn't seem ideal.

@kdungs
Copy link
Contributor Author

kdungs commented Jan 25, 2024

Thanks for taking a look, Greg!

That's a good point, actually. Two things that might be worth considering - also in the interest of not making this a larger refactor (I also thought about adding enum values for the different levels; but again, that would have made this a larger PR):

  1. GSHHS as far as I know isn't actively maintained anymore. The last update is from 2017 (see https://www.ngdc.noaa.gov/mgg/shorelines/gshhs.html). This is only my limited understanding, though so it might be worth checking with the SOEST folks.
  2. The current check has a nice side-effect in that it also functions as a de-facto type check. Since the expected values are integers, callers can't e.g. provide strings. From what I can tell from where it's used in shapereader.py it should be safe. In general, allowing arbitrary user-controlled strings that then go into URLs or filesystem paths can easily lead to security issues 🙈.

So for me personally, the most important aspect is making a minimal change 😄. Then, I think (1) is a good enough argument for keeping it as is for the time being.

What do you think?


Oh and btw. thank you and your team so much for Cartopy. All I wanted to do was quickly slap together a visualization of my friends' and my Antarctica sailing trip and now I'm deep into the rabbit hole of cartography, bathymetry, open data, shapefiles, etc. Even though it eats up considerable amounts of my free time, it's great fun and Cartopy is really nice to work with! 💕

Copy link
Member

@rcomer rcomer left a comment

Choose a reason for hiding this comment

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

The docstring change came in at #1823, which also involved some changes in shapereader.py, including a hard-coded version number. So it looks like, regardless of this check, we would have to do some work if a new version with more levels ever came out.

Given that and @kdungs's points above, I will vote to take this change as is.

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 agree, this is a good update as-is. Thanks for updating this @kdungs, we hope to hear from you again!

@greglucas greglucas merged commit ec891b0 into SciTools:main Jan 25, 2024
6 of 21 checks passed
@kdungs
Copy link
Contributor Author

kdungs commented Jan 25, 2024

Awesome! Thanks a lot, folks!

@QuLogic QuLogic added this to the Next Release milestone Jan 26, 2024
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.

4 participants