Skip to content
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 wfde5 #1991

Merged
merged 16 commits into from Jun 24, 2021
Merged

Cmorize wfde5 #1991

merged 16 commits into from Jun 24, 2021

Conversation

mwjury
Copy link
Contributor

@mwjury mwjury commented Jan 18, 2021

I added a cmorizer for WFDE5 (bias-corrected reconstruction of ERA5). Depends on #1990.

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:

@valeriupredoi
Copy link
Contributor

this looks ready, I added the release tag and we need to fix the iris documentation as per #2011

Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cheers @mwjury - I will merge once we merge the fix to the iris documentation from #2012 🍺

@bouweandela
Copy link
Member

Just applying some new labels to make the status of all open pull requests more clear. Note that the tests on CircleCI and the documentation build must be successful (green) before the label 'approved by technical reviewer' can be applied.

@remi-kazeroni remi-kazeroni mentioned this pull request Feb 9, 2021
9 tasks
Copy link
Contributor

@remi-kazeroni remi-kazeroni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The version of the data mentioned in this PR (v1.0) is now deprecated. @mwjury coud you please update the downloading instrustions, the version number (v1.1) accordlingly and check whether your script runs fine on the newest data? Thanks!

@valeriupredoi
Copy link
Contributor

@remi-kazeroni cheers, mate! Next time please use the review changes button and ask for changes so that the official review is logged and the merge can not happen unless both you as reviewer and the PR creator agree on changes made/not needed πŸ‘

Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technical review passed by me, but changes needed for the sci review from comment

@hb326
Copy link
Contributor

hb326 commented Mar 19, 2021

I think there is a problem with the calculation of the monthly means. I ran the cmorizer and get for the file "OBS_WFDE5_ground_v1.1-CRU+GPCC_Amon_pr_197901-197912.nc" only one time step. I think the monthly means are overwritten when they are stored in the file. Could you check that again?

@hb326
Copy link
Contributor

hb326 commented Apr 9, 2021

@mwjury, you want to check the calculations of the monthly means for this pull request? I am happy to do the scientific review then.

@mwjury
Copy link
Contributor Author

mwjury commented Apr 16, 2021

@hb326 In the previous version for v1.0 the output was monthly files (one month per file), so I do not understand how you got files for a year.
I have updated to the current version, could you please have a look.

@mwjury
Copy link
Contributor Author

mwjury commented Apr 16, 2021

Sorry it took so long. @valeriupredoi this should be ready to go (FLAKE8-check failing due to regex, any ideas).

  • Updated to current version (v1.1) @remi-kazeroni .
  • Accounted for snowfall as raised by @lukruh .
  • Added yearly treatment of files as implemented in cmorize_WFDE5_monthly_fix by @schlunma , removed cmorization of hourly data, but still keeping cmorizing daily data.

Copy link
Contributor

@hb326 hb326 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me the cmorizer looks good now. I think, providing the option to get "pr" out as the sum from rainfall flux and snowfall flux is great! I also get 12 (different) months now in the monthly file, and 365 (different) days for the daily files.

import iris
from cf_units import Unit
from esmvalcore.preprocessor import daily_statistics, monthly_statistics

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I pulled the branch to test it yesterday, this line was only showing up as
from esmvalcore.preprocessor import monthly_statistics

That made the cmorizer crash, of course. I added the daily_statistics manually, and then it ran fine.
This is weird. @valeriupredoi, could you check what you get when you pull the branch?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@valeriupredoi: another question I had last week. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gimme a few, entering another meeting in 10min (unplanned meetings blegh)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's possible you pulled a local copy, or that your git cache may be a bit special 😁 Anyway I pulled the branch from remote and that was there all fine, I now merged master in so all should be tiptop. I'll review it now πŸ‘

@zklaus
Copy link
Contributor

zklaus commented May 5, 2021

(FLAKE8-check failing due to regex, any ideas).

Yes! According to https://www.flake8rules.com/rules/W605.html you should write your regex in a raw string, i.e. r'bla'. The reason is that without the r, Python will replace all occurrences of a backslash followed by something with a control character right then and there, before even passing it to the regex library. So '\n' becomes a newline, '\r' a carriage return, etc. The reason why it still works as it stands is that the backslashes are not followed by any recognized control sequences and therefore are in the end not replaced. This is not completely accidental since back in the days the designers of regular expressions had this in mind, but it might also not be guaranteed, which is why Flake8 is saying it's better to tell Python that you don't want it replacing backslashes here.

@valeriupredoi
Copy link
Contributor

(FLAKE8-check failing due to regex, any ideas).

Yes! According to https://www.flake8rules.com/rules/W605.html you should write your regex in a raw string, i.e. r'bla'. The reason is that without the r, Python will replace all occurrences of a backslash followed by something with a control character right then and there, before even passing it to the regex library. So '\n' becomes a newline, '\r' a carriage return, etc. The reason why it still works as it stands is that the backslashes are not followed by any recognized control sequences and therefore are in the end not replaced. This is not completely accidental since back in the days the designers of regular expressions had this in mind, but it might also not be guaranteed, which is why Flake8 is saying it's better to tell Python that you don't want it replacing backslashes here.

# noqa FTW

Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good stuff @mwjury - couple minor comments from me 🍺

esmvaltool/cmorizers/obs/cmorize_obs_wfde5.py Show resolved Hide resolved
esmvaltool/cmorizers/obs/cmorize_obs_wfde5.py Show resolved Hide resolved
esmvaltool/cmorizers/obs/cmorize_obs_wfde5.py Outdated Show resolved Hide resolved
@zklaus
Copy link
Contributor

zklaus commented May 11, 2021

# noqa FTW

No :( In this case, fix the code. This really should be a raw string. Alternatively, escape your backslashes.

mwjury and others added 2 commits May 18, 2021 10:34
Co-authored-by: Valeriu Predoi <valeriu.predoi@gmail.com>
@mwjury mwjury requested a review from valeriupredoi June 1, 2021 08:06
@valeriupredoi valeriupredoi merged commit 39764f4 into main Jun 24, 2021
@valeriupredoi valeriupredoi deleted the cmorize_WFDE5 branch June 24, 2021 16:02
@remi-kazeroni
Copy link
Contributor

I have just run this cmorizer to put the cmorized data in the obs folder. I realized there is a typo in the WFDE5.yml config file which should have: tier: 2 and not tier: 3. The cmorizer runs fine but the recipe_check_obs.yml not... Sorry, I should have jumped in here earlier and see this while cmorizing the data before this is merged. But in such cases @valeriupredoi please ping me before merging and I'll do my best to have a look asap πŸ‘ The last box was not checked and we agreed that this is my task πŸ˜„ Shall I restore the branch or fix this issue in another PR?

@valeriupredoi
Copy link
Contributor

ouch, totally my bad, sorry - I did notice the last box being unchecked but I thought someone forgot to check it 😁 No need to restore the branch, just do a quick PR with the typo correction and I'll approve it pronto πŸ‘

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants