-
Notifications
You must be signed in to change notification settings - Fork 138
Update CRU CMORizer #3381
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
Update CRU CMORizer #3381
Conversation
…ol into update_cru_cmorizer
Regarding support of multiple version support: The cmorizer is still able to reproduce the same cmorized data for v TS4.02 as before. However, changes to (downloader, ) Regarding codacy errors: Line lengths errors are caused by long urls, which I prefer not to break, but I can if required. The unused arguments seem to be required by the cmorizer class that imports the function. There are several formatters ignoring them, should we consider to make them optional? |
I'm thinking about throwing the number of stations coordinate out again. I don't even know if anyone would use it and it might clash with some simple simple iris operations like merging or substracting different parts of a cube. |
@flicj191 when you have some time, can you look at this please? |
thanks @lukruh, from my experience you don't have to worry about the unused arguments codacy errors, that's been like that for a while. I have used this and downloaded and cmorized cru data on Gadi (Australia). It was picked up fine and comparable in an edited version of |
Hi, we are currently working on the ESMValTool release for v2.11.0. We're wondering if you'd be able to complete this PR by the end of next week (Friday 10th May). Otherwise, please let us know, and we'll move it into the next milestone for you 🙂 |
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 on my side.
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.
Circle CI returns some formatting issues:
./esmvaltool/cmorizers/data/formatters/datasets/cru.py:7:80: E501 line too long (86 > 79 characters)
./esmvaltool/cmorizers/data/formatters/datasets/cru.py:8:80: E501 line too long (86 > 79 characters)
./esmvaltool/cmorizers/data/formatters/datasets/cru.py:9:80: E501 line too long (86 > 79 characters)
./build/lib/esmvaltool/cmorizers/data/formatters/datasets/cru.py:7:80: E501 line too long (86 > 79 characters)
./build/lib/esmvaltool/cmorizers/data/formatters/datasets/cru.py:8:80: E501 line too long (86 > 79 characters)
./build/lib/esmvaltool/cmorizers/data/formatters/datasets/cru.py:9:80: E501 line too long (86 > 79 characters)
Exited with code exit status 1
…ol into update_cru_cmorizer
@rbeucher Thanks for the review! @mo-gill From my side it could be merged, after a second review. If it's already to late feel free to move it to the next mile stone. |
Yes I would avoid breaking them. Technical review. |
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.
Just change the version in the cmorize_obs recipe and all is good. Data in new version looks good, everything runs fine, small changes in recipe outputs, but that's what new versions come with - all scientifically plausible.
@ESMValGroup/obs-maintainers if Levante people want to just move the data from my folder to the public one:
raw: /work/bd0854/b309137/data_ext/RAW/Tier2/CRU
cmorized: /work/bd0854/b309137/data_ext/Tier2/CRU
Co-authored-by: Bettina Gier <[email protected]>
Co-authored-by: Bettina Gier <[email protected]>
Thanks for the review @bettina-gier. Good spot of v4.06 in the test recipe. I downloaded and tested both versions, the most recent one should stay in the test recipe of cause. @rbeucher since you are fine with not breaking the urls, could you withdraw the change request and approve it again? I think with open change request it can not be merged. Thanks a lot. |
@lukruh and @bettina-gier: we have copied the data to the respective folders on Levante. All sorted. |
Description
This is a new approach to update the CRU CMORizer. It includes fixes for the time coordinate, adds the number of stations per cell and provides backwards compatible support for multiple versions of the dataset (TS4.02, TS4.06, TS4.07).
save number of stations via AuxCoord(not compatible to all preproc functions)datasets.yml
(set to newest only)recipe_anav13jclim.yml
recipe_martin18grl.yml
schlund20jgr/recipe_schlund20jgr_gpp_change_rcp85.yml
schlund20jgr/recipe_schlund20jgr_gpp_abs_rcp85.yml
Closes #3380
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.
New or updated data reformatting script
To help with the number of pull requests: