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

Add fix for iris longitude bug to ClimWIP #2107

Merged
merged 5 commits into from
Apr 19, 2021
Merged

Conversation

lukasbrunner
Copy link
Contributor

@lukasbrunner lukasbrunner commented Mar 29, 2021

Description

Implement a fix for a iris longitude problem (ESMValGroup/ESMValCore#800) in the ClimWIP mapplot diagnostic


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 recipe/diagnostic

New or updated data reformatting script


To help with the number of pull requests:

@lukasbrunner lukasbrunner self-assigned this Mar 29, 2021
@lukasbrunner lukasbrunner added bug EUCP www.eucp-project.eu requires new ESMValCore release A new release of ESMValCore is needed to solve this issue/merge this pull request.. labels Mar 29, 2021
@lukasbrunner lukasbrunner marked this pull request as ready for review March 29, 2021 12:32
@Peter9192
Copy link
Contributor

Looks good! Can you have a look at the flake8 errors?

@Peter9192
Copy link
Contributor

Looks good to me. @ruthlorenz as a formality could you do a very quick scientific check?

@ruthlorenz
Copy link
Contributor

What do you need me to check?
I run a quick test using a region crossing the 0 meridian, result attached. But might just be a plotting issue.
temperature_change_weighted_map.pdf

@ruthlorenz
Copy link
Contributor

ruthlorenz commented Mar 31, 2021

no actually the preprocessed files already look like this....
All I changed is the start_longitude in recipe_climwip_basic_check.yml to be -10 (and the xticks accordingly)

@Peter9192
Copy link
Contributor

What do you need me to check?
...
no actually the preprocessed files already look like this....
All I changed is the start_longitude in recipe_climwip_basic_check.yml to be -10 (and the xticks accordingly)

Good catch! I think it's exactly that: you try to use the updated recipe and verify that it provides the results you expected. Sometimes it might also make sense to look at the equations, but in this case that's not really relevant I think.

@lukasbrunner
Copy link
Contributor Author

Good catch @ruthlorenz I had indeed used the wrong default for the antimeridian in the file I pushed. Now it should run. I've also updated the recipe_climwip_test_basic as I guess it is here to test all these cases. So now the are starts at -10 by default to cover that case.

(for regions crossing the -180/180 there is still a problem I would leave it for now and try fix that should we ever need it)

Copy link
Contributor

@ruthlorenz ruthlorenz left a comment

Choose a reason for hiding this comment

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

works like a charm now :-)

@lukasbrunner
Copy link
Contributor Author

Can you merge this @Peter9192? Just to clean up :)

@Peter9192
Copy link
Contributor

I'm not authorized to merge, sorry. You can tag ESMValGroup/esmvaltool-coreteam instead, if you're happy with the docs. I think you might need a blank line below the code block header.

@lukasbrunner
Copy link
Contributor Author

I'm not authorized to merge, sorry. You can tag ESMValGroup/esmvaltool-coreteam instead, if you're happy with the docs. I think you might need a blank line below the code block header.

Will do! Sorry the comment actually belonged to another PR (too many browser windows)

@lukasbrunner
Copy link
Contributor Author

ping @nielsdrost @valeriupredoi :)

@valeriupredoi valeriupredoi merged commit e1d9c67 into master Apr 19, 2021
@valeriupredoi valeriupredoi deleted the fix_mapplot_climwip branch April 19, 2021 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved by scientific reviewer approved by technical reviewer bug EUCP www.eucp-project.eu requires new ESMValCore release A new release of ESMValCore is needed to solve this issue/merge this pull request..
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix ClimWIP mapplot longitude values
4 participants