-
Notifications
You must be signed in to change notification settings - Fork 38
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
Made ICON output UGRID-compliant (on-the-fly) #1664
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1664 +/- ##
==========================================
+ Coverage 92.04% 92.07% +0.02%
==========================================
Files 234 234
Lines 12057 12112 +55
==========================================
+ Hits 11098 11152 +54
- Misses 959 960 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Regarding ...
You can in fact "turn on" the UGRID parsing permanently, by an alternate use of the context manager interface, albeit a bit clunky !
Note, however, that the context object here created must not go out of scope! (since if destroyed, it will turn off again) A simpler but less clean way is just |
it would be quite nice if @bouweandela and @bsolino had a wee time look at this and review it. Cheers 🍺 |
I'll have a look as soon as possible, but last week I was caught up in the IS-ENES3 GA and this week I'm quite busy doing internal stuff for my employer. I will probably have some time later this week. |
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 reviewed the code and documentation and it looks good. I will try running some tests tomorrow.
Co-authored-by: Bouwe Andela <b.andela@esciencecenter.nl>
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 ran some tests with an adapted version of examples/recipe_python.yml
, as shown below, and things seem to work fine, though I'm no expert on UGRIDs.
# ESMValTool
# recipe_python.yml
---
documentation:
description: |
Example recipe that plots a map and timeseries of temperature.
title: Recipe that runs an example diagnostic written in Python.
authors:
- andela_bouwe
- righi_mattia
maintainer:
- schlund_manuel
references:
- acknow_project
projects:
- esmval
- c3s-magic
datasets:
- {project: ICON, dataset: ICON, exp: cool014}
preprocessors:
select_january:
regrid:
target_grid: 0.5x0.5
scheme:
reference: esmf_regrid.experimental.unstructured_scheme:regrid_unstructured_to_rectilinear
method: bilinear
lon_offset: false
extract_month:
month: 1
diagnostics:
map:
description: Global map of temperature in January 2000.
themes:
- phys
realms:
- atmos
variables:
tas:
mip: Amon
preprocessor: select_january
start_year: 1980
end_year: 1980
scripts:
script1:
script: examples/diagnostic.py
quickplot:
plot_type: pcolormesh
cmap: Reds
I have read the code and all looks good :) I've had a small issue when running the test that I'm not sure if it's on my end: It is looking for the ICON grid on my personal folder |
Thanks for reviewing @bouweandela and @bsolino! After a quick offline discussino with @bsolino we found that the cache directory is not automatically created when it's not present, which leads to the error Brei mentioned. This is fixed now via a simple |
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.
Everything looks good now, good work!
Description
This is a first suggestions of an on-the-fly ICON -> UGRID converter for the ICON CMORizer, which eventually will allow us to use more sophisticated regridding algorithms (e.g., from iris-esmf-regrid) on ICON data.
I successfully tested this using the area weighted scheme from iris-esmf-regrid (I had to make some other modifications to the code, though; see below), which gave very similar results to CDO's
remapcon
:Difference between CDO and iris-esmf-regrid for tas
Closes #1653
Link to documentation: https://esmvaltool--1664.org.readthedocs.build/projects/ESMValCore/en/1664/quickstart/find_data.html#icon
Issues that need to be solved before merging this
iris-esmf-regrid does not yet offer a--> not yet necessary anymore due to Add iris-esmf-regrid as a dependency #1809.iris.cube.Cube.regrid()
-compatible scheme for their mesh regridder (I temporarily added this to test it) - seeiris.cube.Cube.regrid()
-compatible scheme forMeshToGridESMFRegridder
SciTools/iris-esmf-regrid#192.MeshCoord
added to the cube usingMesh.to_MeshCoords
always uses the metadata of the node coordinate and always usesvar_name=None
, which leads to errors in our CMOR checks, e.g.,Coordinate longitude has var name None instead of lon
-MeshCoord
constructor always uses metadata of node coordinate regardless oflocation
SciTools/iris#4860.Related issues (would be nice to solve these before merging, but not crucial)
iris.experimental.ugrid.PARSE_UGRID_ON_LOAD
has to be used. Thus, diagnostics cannot read this data out-of-the box. I guess this will be removed by iris once meshes are fully supported and lose their "experimental" status.After this PR
main
and fix it (see Updated URL of ICON grid file used for testing #1914)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: