-
Notifications
You must be signed in to change notification settings - Fork 44
Support IPCC AR6 regions in extract_shape
#2008
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2008 +/- ##
==========================================
+ Coverage 93.02% 93.06% +0.03%
==========================================
Files 237 237
Lines 12706 12743 +37
==========================================
+ Hits 11820 11859 +39
+ Misses 886 884 -2
|
valeriupredoi
left a comment
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.
quick comment through which I am signing myself up as a reviewer for this (cheers, Manu, very good idea!) but I got a meeting in 5min so I'll be back here after that 🍺
|
I'll review this in a jiffy, today |
valeriupredoi
left a comment
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.
this is very cool and very useful too - heard a few times about the need of AR6 regions in ESMValTool at the TerraFIRMA GA two weeks ago, very much cheers, Manu! I am approving because I don't want to be the ned stopping progress but I have a couple more serious questions (other than typos and minor code suggestions):
- how big are those shapefiles?
- I'd stay away from decalring types that may change in the future even if the external API may stay the same (eg fiona.collection.Collection)
Cheers, bud 🍺
|
Thanks V, I addressed all your comments in 8847b95! Regarding your questions:
Thanks 🍻 |
|
brilliant, you're a star, Manu! Ready to be merged then 🥳 |
bouweandela
left a comment
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 @schlunma, great addition!
|
then @bouweandela can merge, we found us a merger too 👍 |
Description
This PR refactors the
extract_shapepreprocessor and makes it more flexible, bydictfor theidsparameter to extract regions based on a non-default attribute name (e.g.,Acronymfor the IPCC AR6 regions). Default names areidandname(with different capitalization options).shapefile: ar6.Example:
Closes #1648
Closes #758
Related to #2007
Link to documentation:
Before you get started
Checklist
It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.
To help with the number pull requests: