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 mapplot diagnostic to ClimWIP #1864

Merged
merged 23 commits into from Nov 4, 2020
Merged

Add mapplot diagnostic to ClimWIP #1864

merged 23 commits into from Nov 4, 2020

Conversation

lukasbrunner
Copy link
Contributor

@lukasbrunner lukasbrunner commented Oct 15, 2020

Tasks

  • Create an issue to discuss what you are going to do, if you haven't done so already (and add the link at the bottom)
  • Give this pull request a descriptive title that can be used as a one line summary in a changelog
  • Make sure your code is composed of functions of no more than 50 lines and uses meaningful names for variables
  • Circle/CI tests pass. Status can be seen below your pull request. If the tests are failing, click the link to find out why.
  • Preferably Codacy code quality checks pass, however a few remaining hard to solve Codacy issues are still acceptable. Status can be seen below your pull request. If there is an error, click the link to find out why. If you suspect Codacy may be wrong, please ask by commenting.
  • Please use yamllint to check that your YAML files do not contain mistakes

Modified recipe/diagnostic

  • Update documentation for the recipe to the doc/sphinx/source/recipes folder
  • Assign the author(s) of the affected recipe(s) as reviewer(s)

Closes #1863

@lukasbrunner
Copy link
Contributor Author

Example output:
temperature_change_weighted_map
temperature_map_difference

@lukasbrunner lukasbrunner self-assigned this Oct 15, 2020
@lukasbrunner lukasbrunner added the EUCP www.eucp-project.eu label Oct 15, 2020
@lukasbrunner lukasbrunner marked this pull request as ready for review October 15, 2020 13:31
Copy link
Contributor

@Peter9192 Peter9192 left a comment

Choose a reason for hiding this comment

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

Hey Lukas!
Nice work on implementing this and passing all checks (codacy is okay)! I haven't tested it yet, but I had a quick look at the code. I left a few comments/ideas, hope to have a closer look at it later.

One small side note is that the figure headers weren't immediately obvious to me, I think because the second one has two 'minuses'. perhaps you could call it '(weighted minus unweighted) temperature anomalies' ?

@lukasbrunner
Copy link
Contributor Author

Thanks for the comments @Peter9192! I agree that a shared utils file could make sense, in fact I started from your plotting script so a lot of the basic functions are identical. I will start to restructure this.

About the anomalies preprocessor: can you write down the syntax for this? I failed and @ruthlorenz also did not know how to do that. You use it for the timeseries plot but there the reference period is in the period you then also plot. I need to calculate two climatologies in non-overlapping time periods and subtract them from each other.

@Peter9192
Copy link
Contributor

About the anomalies preprocessor: can you write down the syntax for this?

Not tested, but I think it should be something like:

preprocessors:
  tas_anomalies:
    custom_order: true
    regrid ... # with appropriate keywords
    anomalies: 
      period: full  # this means substracting the overall mean of period below.
      reference:  # this is your reference period, i.e. the climate you want to substract from the other one
        start_year: 1981
        start_month: 1
        start_day: 1
        end_year: 2010
        end_month: 12
        end_day: 31
      standardize: false  # whether you want the result in K or in % (or decimal, not sure what it gives).
    # So far you now still have the full period, but only relative to this reference period. So now you can continue:
    extract_time: 
        start year: 2081
        start .... # etc.
    climate_statistics:
      operator: mean
    

diagnostics:
  variables:
    tas_map:
      short_name: tas
      preprocessor: tas_anomalies
      start_year: 1981  # this can be defined here, or in the datasets section. 
      end_year: 2100  # But this span needs to encompass both reference and target period.

So to summarize:

  1. average over reference period
  2. substract that average from all other times (all this is done by the anomalies preprocessor)
  3. (extract and) average over the target period

I think you're description is swapping steps 2 and 3, but I don't think that makes a difference.
Does that help?

@bouweandela
Copy link
Member

The preprocessor above may need ESMValGroup/ESMValCore#796 to work.

@Peter9192
Copy link
Contributor

The preprocessor above may need ESMValGroup/ESMValCore#796 to work.

Good point. So maybe the last 2 steps can be done in the diagnostic script until then.

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.

Looks good to me in general, a few suggestions in my comments.
The only thing that really needs resolving is the set up of the recipe and if the required python version can be upgraded easily.

doc/sphinx/source/recipes/recipe_climwip.rst Show resolved Hide resolved
esmvaltool/recipes/recipe_climwip.yml Show resolved Hide resolved
@lukasbrunner
Copy link
Contributor Author

I've fixwd both issues @ruthlorenz. The assignment expressions were not really necessary and without them it should be okay to run under 3.6.

@lukasbrunner
Copy link
Contributor Author

@stefsmeets @Peter9192 could one of you have a quick look at this again and authorize the merge (if everything looks good)?

@Peter9192
Copy link
Contributor

Yes, I absolutely want to get this merged! I'll try to get to it today, but no promises...

Copy link
Contributor

@Peter9192 Peter9192 left a comment

Choose a reason for hiding this comment

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

Hey @lukasbrunner very nice work. The code works, and already look very good. I have a couple of suggestions. Could you have a look at them?

Please also have a look at the contributing guidelines. We try to format the code consistently, by using yapf and isort. They are both quite easy to use. If you want to integrate all checks and auto-formatting, you can use pre-commit. It's quite useful once you get used to it.

lukasbrunner and others added 2 commits October 23, 2020 16:19
Co-authored-by: Peter Kalverla <peter.kalverla@gmx.com>
Co-authored-by: Peter Kalverla <peter.kalverla@gmx.com>
@lukasbrunner
Copy link
Contributor Author

lukasbrunner commented Oct 28, 2020

Slowly getting there :) This might be the wrong place for this discussion but since @bouweandela raised it: It does not seem to make sense to import xarray just to use in an annotation? What is the preferred solution here?

undefined name 'xr' (F821)
def visualize_and_save_temperature(temperature: 'xr.DataArray', cfg: dict,

Update: okay now I added it and it complains that it is unused...

@stefsmeets
Copy link
Contributor

Slowly getting there :) This might be the wrong place for this discussion but since @bouweandela raised it: It does not seem to make sense to import xarray just to use in an annotation? What is the preferred solution here?

undefined name 'xr' (F821)
def visualize_and_save_temperature(temperature: 'xr.DataArray', cfg: dict,

Update: okay now I added it and it complains that it is unused...

We had this discussion before when developing the recipe. I'm a big fan of type annotations and use them whenever I can. There is nothing wrong with the code here. It is just that flake8 and CircleCI do not like it one way or the other and (even worse) do not agree with each other what is right. I think in the end we had to use xarray somewhere, so we could get around the 'unused variable' issue. If you cannot find a way to use xarray somewhere, best to avoid the issue altogether and remove the annotation to keep the CI/flake8 happy.

@bouweandela
Copy link
Member

This might be the wrong place for this discussion but since @bouweandela raised it: It does not seem to make sense to import xarray just to use in an annotation? What is the preferred solution here?

I think this is a very good place to ask this question. The solution of not using type annotations is absolutely fine, but if you want to use them, you need to import it, because otherwise there is no way of telling what you mean with xr. This is a bug in pylint, reported in several issues, e.g. this one: pylint-dev/pylint#3299.

@bouweandela
Copy link
Member

@esmvalbot recipe_climwip.yml

@esmvalbot
Copy link

esmvalbot bot commented Oct 29, 2020

Starting recipe recipe_climwip.yml, output will be generated here

@esmvalbot
Copy link

esmvalbot bot commented Oct 29, 2020

Bot failed to run recipe recipe_climwip.yml: exit is 1, output has been generated here

@Peter9192
Copy link
Contributor

Bot failed to run recipe recipe_climwip.yml: exit is 1, output has been generated here

@bouweandela I like the test case for the new bot, but let's try to solve that issue somewhere else. I tested the recipe locally during the technical review, and without the missing data the recipe runs fine. Let's get this merged so Lukas can continue with follow-up PRs.

@bouweandela
Copy link
Member

@esmvalbot recipe_climwip.yml

@esmvalbot
Copy link

esmvalbot bot commented Oct 29, 2020

Starting recipe recipe_climwip.yml, output will be generated here

@esmvalbot
Copy link

esmvalbot bot commented Oct 29, 2020

Bot failed to run recipe recipe_climwip.yml: exit is 1, output has been generated here

@bouweandela
Copy link
Member

bouweandela commented Oct 29, 2020

Ok, I changed the configuration of the bot so it now finds all the data, but now it's failing on something that looks related to the cartopy version, SciTools/cartopy#1615. I'll have a look.

@Peter9192
Copy link
Contributor

Ok, I changed the configuration of the bot so it now finds all the data, but now it's failing on something that looks related to the cartopy version, SciTools/cartopy#1615. I'll have a look.

That's the same problem we had with the example recipe in #1882.

@Peter9192
Copy link
Contributor

@bouweandela can you please merge this? Let's solve the bot issue somewhere else (e.g. #1882) and not block Lukas' work on it.

@bouweandela
Copy link
Member

Let's have a final try to get it working, the current failure is not related to the bot, but to the wrong versions of dependencies, so at the moment this recipe will not work for anyone who does a new installation. I think this can be fixed by #1898. Once we have that merged into master, we can merge master into this branch and have a final go at running it with the bot.

@bouweandela
Copy link
Member

@esmvalbot recipe_climwip.yml

@esmvalbot
Copy link

esmvalbot bot commented Nov 4, 2020

Starting recipe recipe_climwip.yml, output will be generated here

@esmvalbot
Copy link

esmvalbot bot commented Nov 4, 2020

Recipe recipe_climwip.yml ran OK, output has been generated here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
diagnostic EUCP www.eucp-project.eu
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a mapplot diagnostic to ClimWIP
6 participants