-
Notifications
You must be signed in to change notification settings - Fork 168
Add FieldSet.from_sgrid_conventions()
#2432
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 FieldSet.from_sgrid_conventions()
#2432
Conversation
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.
Looks good! A few comments/questions below
| See https://sgrid.github.io/ for more information on the SGRID conventions. | ||
| """ | ||
| ds = ds.copy() |
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.
Why is it necessary to make a copy?
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 a shallow copy - from_fesom2 and from_copernicusmarine do them both to avoid changes to the dataset propogating to the original dataset
|
|
||
| fields = {} | ||
| if "U" in ds.data_vars and "V" in ds.data_vars: | ||
| fields["U"] = Field("U", ds["U"], grid, XLinear) |
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.
But this interpolator may not be the default for nemo; so how would from_nemo overwrite the the interpolator here?
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.
I think it should be the default no? Since XLinear would be aware of the grid positioning?
Otherwise we would need to expose this as a param to the function
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.
No, XLinear is only the default for an A-grid layout. For a C-Grid layout, the velocities should be CGrid_Velocity and any other field should be CGrid_Tracer. SO it depends on the Grid topology
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.
Discussed in person. Outcome: I think we can find where the variables are on the grid via the sgrid conventions, and then use that to choose the interpolator. We'll merge for now, I can rework from_copernicusmarine to use this new code path, and we can explore in future how we can specify this in SGRID and adapt our interpolators
Nodes and edges were the wrong way around
In node_dimensions its X then Y axis
This method ingests datasets using SGRID convention metadata alongside tests. This is a first iteration and can be improved in future
I think this is better to be explicit that this isn't the execution path for ugrid conventions. Down the line we could provide a `from_conventions` - but at the moment I say we shouldn't (since its not clear how uxarray handles metadata).
d528f03 to
5d7c8fa
Compare
Towards #2003
Changes:
The testing as it stands for
from_conventionsis just testing init on the datasets provided.I think there's room for future improvements (which I've put as comments in the body)
This acts as the base for future methods (
from_nemoetc) for ingesting structured data. Here is what I have in mind: