-
Notifications
You must be signed in to change notification settings - Fork 168
Add submodule parcels._core.sgrid and tests
#2418
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
Add submodule parcels._core.sgrid and tests
#2418
Conversation
Happy to just add this to the agenda for our next meeting - that way I can give you a rundown on SGRID if you need it. |
d8fa50d to
ce9d428
Compare
664d35e to
649925a
Compare
|
Ok, did a self review. +600 lines seems like a large diff, but most of the lines are in the SGRID data model/Hypothesis strategies sections - this shows how the code relates to the standard which is really useful. @erikvansebille keen to have your review (happy to do async, or in person on Wednesday if you prefer) Do you want me to also work on ingestion code (i.e., |
48be6ed to
5161b3f
Compare
erikvansebille
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 @VeckoTheGecko, just did a first review. I'm not sure I understand the concept of a DimDimPadding, perhaps good to talk that through. Otherwise looks good, with a few suggestions below
Mainly as developer documentation, so its clear what the input and output should be
Co-authored-by: Erik van Sebille <[email protected]>
2ba0d84 to
fc99fa2
Compare
erikvansebille
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.
OK, all good then. One minor type-o fix below
|
Thanks! Merging on green 🚀 |
SGrid acts as a standard that we can use to encode information about grid staggering etc. in the metadata.
I found the logic for Sgrid parsing in xgcm (https://github.com/xgcm/xgcm/blob/c9d4302b146d070c099a833cab853157539d01f7/xgcm/sgrid.py ) to be very difficult to follow and unnecessarily complicated (with limited testing).
Rather than rely directly on this parsing in Xgcm, I decided to create our own Sgrid convention parsing for the following reasons:
from_mitgcmetc. flow through afrom_conventionsclassmethod). The functions and tooling in xgcm isn't easily adaptable for the serializing of sgrid metadata.cc @jbusecke (drop a comment if you agree that this sgrid parsing is more readable - and we can see about eventual upstreaming into xgcm)
Update
This PR is now ready for review.
src/parcels/_core/sgrid.py: Adds data structures for SGRID metadata that closely follow the convention, as well as tooling for serializing/deserializing to and from attrs dictionaries and these objects. This allows you validate that the SGRID metadata is valid, and navigate the resulting metadata with rich type hints. The focus here has been on correctness, broken metadata will cause the parser to informatively fail - this is a feature (see appendix for a bit of nuanced discussion). The focus here has also been on the required attributes (andvertical_dimensionsin the 2D case) as that is what is required in Parcels.tests/strategies/sgrid.py: These provide various Hypothesis test strategies related to generating SGRID related metadata, including strategies for the metadata objects themselvestests/test_sgrid.py: Provides testing of SGRID metadata parsing and testing of ingestion of this metadata intoxgcm.Gridobjects. This heavily relies on property based testing as its best suited here.The components introduced here are modular - allowing for the potential upstreaming into other libraries and projects.
Example usage
output:
appendix
There is two kinds of users who would want to parse SGRID data:
I opted to write this code leaning towards (1) for the following reasons:
from_nemoetc. methods