-
Notifications
You must be signed in to change notification settings - Fork 128
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
Cmorize cru v4.04 & stn contraint #1981
Conversation
…Group/ESMValTool into cmorize_cru_stn_contraint
#1980 causing failed tests |
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.
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.
Nice work @mwjury! This is almost ready to be merged I think. Please just merge the master
branch into this branch and extend the time_range to 2019 in the recipe_check_obs.yml
. We could merge this PR after that.
This seems to be pretty much done, right? @remi-kazeroni fixed the end year to 2019, and #2021 seems to be merged, right @valeriupredoi? |
@valeriupredoi: and this was one of the PR I wanted you to comment on. |
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 to me, I am just a bit quizzical about a relatively involved function that could be as simple as changing all days to the 15th of the month?
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 works fine for me, thanks @mwjury!
This is another example where updating the cmorizer would delete the previous version (c.f. #2189). This case is a but simpler because the cmorizer script has not changed much (but the fix for the time coordinate differs between the 2 versions). So I'm not sure what is optimal between having:
Besides, we have a few recipes that are using the version I'd preferto solve that issue before merging this PR. |
How to continue with this PR? |
I would recommend to this close this PR. It is outdated and still based on the old CMORizer interface. We could keep the branch for some time if it helps for future developments. Instead, you could open a new PR from latest |
Dear @remi-kazeroni thanks a lot for your answer! Because there are several recipes using CRU and because the data set is not too large (monthly data, limited number of variables) I think it would be best to support two versions in parallel. I will have a look at the the WOA CMORizer. We should keep the branch at the moment, because in addition to the version change it contains a fix for an error in the time coordinate and I first need to check if that is relevant for CRU 4.02 and 4.07 as well. |
I checked the CRU (TS) v4.06 and v4.07 and the time coordinate are not exactly centered. They are set to 16th 00:00:00 of each month (15th for februrary). Fixing them would provide exact boundaries and centered points. If this cause any changes in the results of the mentioned recipes they should be small. However, to be sure not to break them we could add the fix for v4.06+ when approaching a multi version CMORizer as in WOA. Should the download script also support multiple versions? Another thing that seems to be missing in the current |
As the PR for latest CRU Versions is merged now (including the time fix) I think this PR can be closed. @mwjury? |
Description
Checklist
It is the responsibility of the author to make sure the PR 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: