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 hadcrut5 #1977

Merged
merged 13 commits into from Feb 17, 2021
Merged

Cmorize hadcrut5 #1977

merged 13 commits into from Feb 17, 2021

Conversation

mwjury
Copy link
Contributor

@mwjury mwjury commented Jan 13, 2021

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:

I added a cmorizer for the new HadCRUT5 dataset.

Any suggestion on the protected function issues?

@valeriupredoi
Copy link
Contributor

documentation tests failed because numpy was offline and the test tests fail coz of this #1980 - I'll have a look at the code later but don't worry about the tests!

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 - a small remark about using the utils module; I will merge once we merge the fix to the iris documentation from #2012 🍺 Also, question about all your cmorizers that you have written so far - and great many thanks, awesome contributions - did you produce the cmorized data and alerted @remi-kazeroni of the new data to be uploaded on DKRZ (I imagine not JASMIN since I've not heard of any new data coming there). It's good to get in this habit, since this saves the future user the chore of cmorizing it themselves πŸ‘

if cube_coord.points[0] < 0. and \
cube_coord.points[-1] < 181.:
cube_coord.points = \
cube_coord.points + 180.
Copy link
Contributor

Choose a reason for hiding this comment

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

just use utils.fix_coords(cube) for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with fix_coords is that it calls _fix_bounds on the time coordinate -> removing and reguessing bounds and thereby screwing up the monthly bounds via its linear interpolation ...

Copy link
Contributor

Choose a reason for hiding this comment

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

aha good pointer! I'll put in a patch to utilities.fix_coords() via an optional argument to keep the bounds untouched πŸ‘

Copy link
Contributor

Choose a reason for hiding this comment

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

note that now we have a a set of new args to this function that allow the developer to turn off overwriting the bounds, so next time it's best to use that. It's fine here, no need to change the code πŸ‘

@mwjury
Copy link
Contributor Author

mwjury commented Feb 4, 2021

@valeriupredoi utils.fix_coords(cube) not gonna work unfortunately.
I have added @remi-kazeroni to the todo list when I opened the pull request. Not sure what else to do in this regard. I am using ESMValTool currently neither on DKRZ nor CEDA, but on my institute's infrastructure.

@remi-kazeroni remi-kazeroni self-requested a review February 4, 2021 16:01
@remi-kazeroni
Copy link
Contributor

I have added @remi-kazeroni to the todo list when I opened the pull request. Not sure what else to do in this regard. I am using ESMValTool currently neither on DKRZ nor CEDA, but on my institute's infrastructure.

I suggest that I could do a final review this PR and test the cmorizer at DKRZ: data download and cmorizer. If things are fine I'll approve and put the data in the right directories on DKRZ. I'll do that for all your cmorizer PRs ready to merge before the code freeze. Is that fine @mwjury @valeriupredoi ?

@valeriupredoi
Copy link
Contributor

totally awesome, cheers @remi-kazeroni for that and @mwjury for the code!

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.

Nice work @mwjury ! The cmorizer and the check_obs recipe work fine for me. It uploaded the cmorized data on Mistral.

The only thing is that a later version of the data is now available instead of what you documented. Could you please change from 5.0.0.0 to 5.0.1.0 (or use wildcards: 5.*.*.*) in the documentation and the config file? And replace 2018 by 2020 in the recipe_check_obs.yml? And perhaps merge the master branch to resolve the failing tests? Thanks!

@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
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

4 participants