-
Notifications
You must be signed in to change notification settings - Fork 168
Implementing convert.nemo_to_sgrid()
#2460
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
base: v4-dev
Are you sure you want to change the base?
Implementing convert.nemo_to_sgrid()
#2460
Conversation
And update of tutorial_nemo_curvilinear
And updating tutorial
ANd expading comments on loading in the dataset
To use same ordering as in XLinear
This requires smaller selection_dict for the isel, so hopefully faster code
And also separating offset calculation into its own helper function
It's not clear why this is here, nor why removing it causes a test failure. To be investigated another time. EDIT: This was introduced in Parcels-code@c311fba - though we're investigating if this can be implemented another way since there should be no particular difference with NEMO
Gradually reducing the dependency on the `mesh` param
Updates the API for conversion to be more closely aligned with the input data. Also handles the U and V fields separately - correctly assigning the dimension naming before merging into a single dataset.
Update tests
Avoid assuming there's a U and V field. Maybe this should be refactored later...
6d12f21 to
0b9c787
Compare
|
I'll do a full self review next week. Feel free to take a look in the meantime @erikvansebille if you'd like |
|
Also still a few failing tests that I need to sort out... |
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 nice already. Some first comments below
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 and other tutorials still need to be updated to use the new convert workflow? Do you want me to do that? Could be a good test-case how user-friendly the convert functions are
| :titlesonly: | ||
| examples/explanation_grids.md | ||
| examples/tutorial_nemo_curvilinear.ipynb | ||
| examples/tutorial_nemo_3D.ipynb |
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 the file is at examples_v3/tutorial_nemo_3D.ipynb so it either has to be moved (which then destroys the git diff), or we use the old directory here and then update it in a later PR?
| examples/tutorial_nemo_3D.ipynb | |
| examples_v3/tutorial_nemo_3D.ipynb # TODO move to examples folder |
| return _metadata_rename_dims(self, dims_dict) | ||
|
|
||
| def get_value_by_id(self, id: str) -> str: | ||
| """In the SGRID specification different parts of the spec are identified to with different "ID"s. |
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.
| """In the SGRID specification different parts of the spec are identified to with different "ID"s. | |
| """In the SGRID specification for 2D grids, different parts of the spec are identified by different "ID"s. |
| return _metadata_rename_dims(self, dims_dict) | ||
|
|
||
| def get_value_by_id(self, id: str) -> str: | ||
| """In the SGRID specification different parts of the spec are identified to with different "ID"s. |
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.
| """In the SGRID specification different parts of the spec are identified to with different "ID"s. | |
| """In the SGRID specification for 3D grids, different parts of the spec are identified by different "ID"s. |
| vector_interp_method=CGrid_Velocity, | ||
| # ^Seems to work with AGrid as well? (at least, tests aren't failing - | ||
| # either logic needs to be added to choose interpolator, or this interpolator should be renamed) |
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.
| fields["W"] = Field("W", ds["W"], grid, XLinear) | ||
| fields["UVW"] = VectorField("UVW", fields["U"], fields["V"], fields["W"]) | ||
| fields["UVW"] = VectorField( | ||
| "UVW", fields["U"], fields["V"], fields["W"], vector_interp_method=CGrid_Velocity |
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.
Also here, it would depend on the grid topology whether we use XLinear_Velocity or CGrid_Velocity
| if mesh == "spherical": | ||
| if "U" in fieldset.fields: | ||
| fieldset.U.units = GeographicPolar() | ||
| if "V" in fieldset.fields: | ||
| fieldset.V.units = Geographic() |
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 would all go if we implement #2459. We need to be careful when fixing merge conflicts...
| These functions use knowledge about the model to attach any missing metadata. The functions | ||
| emit verbose messaging so that the user is kept in the loop. The returned output is an | ||
| Xarray dataset so that users can further provide any missing metadata that was unable to | ||
| be determined. |
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.
| be determined. | |
| be determined before they pass it to the FieldSet constructor. |
| } | ||
|
|
||
|
|
||
| def _maybe_bring_UV_depths_to_depth(ds): |
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.
Would all the functions in this convert file require verbose logging? Or is that too much?
| pytest.param( | ||
| AdvectionRK4_3D, | ||
| marks=pytest.mark.xfail( | ||
| reason="from_nemo had 'fieldset.V.units = GeographicPolar()', I'm not sure _why_ this code is needed to get this to pass. To be further investigated.", |
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 PR supercedes #2447
implementing the following additional functionality:
FieldSet.from_nemo()functionality into aconvert.pymodule with anemo_to_sgrid()function. Also added corresponding tests.nemo_to_sgridfunction more closely align with the input datasetsFieldSet.from_sgrid_conventionsto infermeshfrom the metadata included in the datasetget_value_by_id()function toGrid*DMetadataclasses allowing to easily refer to parts of the SGRID metadata modelFuture work:
convert.py- its important that we refactor this module so that its clearer, and have logging so that changes to the input dataset are communicated to the user. To support this, we needI'll make an issue to track this future work.