Esmvaltool: Arctic Ocean diagnostics

[koldunovn]

Short description of the diagnostic
The main focus of this diagnostics is evaluation of ocean components of climate models in the Arctic Ocean, however most of the diagnostics are implemented in a way that can be easily expanded to other parts of the World Ocean. Most of the diagnostics aim at model comparison to climatological data (PHC3), so we target historical CMIP simulations. However scenario runs also can be analysed to have an impression of how Arcti Ocean hydrography will chnage in the future.

Branch and pull request
Branch: https://github.com/ESMValGroup/ESMValTool/tree/applicate_trr
PR: https://github.com/ESMValGroup/ESMValTool/pull/721

[koldunovn]

Working on the codacy issues.

[koldunovn]

@bouweandela Should I address Too many local variables and Too many arguments or just try to reduce the function to 50 lines?

[koldunovn]

@zklaus Can we, please, agree on the review decision?

[koldunovn]

@bouweandela In plotting functions, I use names of the variables like fig, ax, cb, cs. This is basically a standard in python plotting code, and make things clear for those who use matplotlib a lot. The 'codacy' naturally complains about the length of the variables. Do you think I still have to change them?

[bouweandela]

Should I address Too many local variables and Too many arguments or just try to reduce the function to 50 lines?

Preferably you should address them, but if you have something like 10 or so left it's fine. Reducing the functions to about 50 lines is a great rule of thumb, because that means you can see the entire function without scrolling, which really helps to avoid unseen bugs.

In plotting functions, I use names of the variables like fig, ax, cb, cs. This is basically a standard in python plotting code, and make things clear for those who use matplotlib a lot. The 'codacy' naturally complains about the length of the variables. Do you think I still have to change them?

I think it would be good to stick with the Codacy advice. I think names like figure, axis, colorbar would be nicer for those who are not intimately familiar with matplotlib and just as easy to understand for those who do use matplotlib a lot.

[koldunovn]

Hi @bouweandela, @zklaus, @mattiarighi

Here is the status. I think I ticked all the boxes:

• provenance is added to all generated files.
• There are only 11 codacy issues left, among them, two yellow ones seem to be codacy mistakes:
  
  
  
  • interpolate_esmf has only 18 variables (at least VSCode thinks so), not 31, anyway the function is quite short now.
  
  
  • "Unused argument 'kwargs'" for genfilename seem to be a pylint issue https://github.com/PyCQA/pylint/issues/2214
  
  
• Most of the rest of the codacy issues related to too many local arguments in plotting functions. I don't see how to make them shorter without doing unreadable (to me at least). Moreover looking at the plotting functions in other diagnostics I can see that long plotting functions seem to be tolerated.
• CI tests are all green.

[koldunovn]

@zklaus Thank you for the comments. I have implemented changes you requested and remove cmorization part, I will do a separate PR with it.

@bouweandela @mattiarighi Can you, please, comment if something else should be done?

[mattiarighi]

I'll test the PR once @bouweandela has approved it, as usual.

[koldunovn]

I have updated the environment and fx files do not work anymore. Looking at the issue. Maybe @valeriupredoi can give a quick hint of where to look. I try to get areacello variables like this:

areacello:
        project: CMIP5
        mip: fx
        exp: historical
        ensemble: r0i0p0
        start_year: 2000
        end_year: 2002

and when ESMValTool tries to get areacello from climatology, that has only one time step and defined like this:

additional_datasets:
      - {dataset: PHC,  project: OBS6,  tier: 2, type: clim, version: 3, start_year: 1950,  end_year: 1950}

It seems to try to concatenate two areacello files and naturally fails with:

ValueError: Can not concatenate cubes.

If it helps the names of climatological files are:

OBS6_PHC_clim_3_Omon_so_195001-195012.nc
OBS6_PHC_clim_3_Omon_thetao_195001-195012.nc
OBS6_PHC_clim_3_fx_areacello.nc

And the complete recipe can be found here:
https://github.com/ESMValGroup/ESMValTool/blob/applicate_trr/esmvaltool/recipes/recipe_arctic_ocean.yml

[valeriupredoi]

@koldunovn I am running your recipe without the PHC data (I dont have it, nor is it on Jasmin/esmeval) and am not noticing any concatenation errors: INFO Successfully completed task arctic_ocean/areacello (priority 0) in 0:01:25.616730 - can you pls tell me which files it is trying to concatenate and it's failing? :beer:

[koldunovn]

Ok, the problem is solved, I had two areacello files in the directory.

But now I have another one :) Related to https://github.com/ESMValGroup/ESMValTool/pull/1459 As suggested by @mattiarighi I changed mip for observations to Oyr. Now I am in the situation when my data (thetao, so) have mip: Omon , areacello data have mip: fx, and my additional dataset (PHC) has mip: Oyr.

• If I don't specify mip in the additional dataset, the code searches for OBS6_PHC_clim_3_Omon_thetao[_.]*nc (observations with mip: Omon, while they have mip: Oyr)
• If I add mip: Oyr to additional dataset, then the code searches for OBS6_PHC_clim_3_Oyr_areacello[_.]*nc, while areacello file should have mip: fx.

The easiest option would be to revert changes in https://github.com/ESMValGroup/ESMValTool/pull/1459 and keep mip: Omon for climatology data. But maybe there is a better option (ping @valeriupredoi ).

Complete recipe:
https://github.com/ESMValGroup/ESMValTool/blob/applicate_trr/esmvaltool/recipes/recipe_arctic_ocean.yml

[koldunovn]

@koldunovn I am running your recipe without the PHC data (I dont have it, nor is it on Jasmin/esmeval) and am not noticing any concatenation errors: INFO Successfully completed task arctic_ocean/areacello (priority 0) in 0:01:25.616730 - can you pls tell me which files it is trying to concatenate and it's failing? 🍺

@valeriupredoi thanks for the answer! Sorry, it was a stupid mistake of having two areacello files in one directory (in subdirectory actually).

[valeriupredoi]

this is why I tried to convince @mattiarighi and @bouweandela that my proposed priority assignment of mip should be: if mip specified in variable then that should be prioritized over the mip specified in the dataset dictionary! This case here is not solved without such a priority assignment; the only way around it in a nasty way is to make the PHC data Omon

[koldunovn]

this is why I tried to convince @mattiarighi and @bouweandela that my proposed priority assignment of mip should be: if mip specified in variable then that should be prioritized over the mip specified in the dataset dictionary! This case here is not solved without such a priority assignment; the only way around it in a nasty way is to make the PHC data Omon

For me, it's the easiest solution, so I am happy :) Thanks for looking into this!

[valeriupredoi]

okeydokey :grin:

[koldunovn]

@bouweandela is there a chance that you will have time to look at the PR in the near future? 😳

[bouweandela]

@koldunovn My apologies for the slow reply. I just reviewed the code.

[bouweandela]

The easiest option would be to revert changes in #1459 and keep mip: Omon for climatology data. But maybe there is a better option

Yes, there is a better option: you can specify the additional datasets per variable:

    variables:
      areacello:
        project: CMIP5
        mip: fx
        exp: historical
        ensemble: r0i0p0
        additional_datasets:
          - {dataset: PHC, project: OBS6, tier: 2, type: clim, version: 3}
      thetao: &variable
        mip: Omon
        project: CMIP5
        exp: historical
        ensemble: r1i1p1
        start_year: 2002
        end_year: 2005
        additional_datasets:
          - {dataset: PHC, project: OBS6, mip: Oyr, tier: 2, type: clim, version: 3, start_year: 1950, end_year: 1950}
      so: *variable

The thing to keep in mind is that files are defined using a combination of the keys in the variable section and dataset section, where keys from the dataset section take precedence. This is also explained in the documentation: https://esmvaltool.readthedocs.io/projects/esmvalcore/en/latest/esmvalcore/recipe.html#variable-and-dataset-definitions

I also used a yaml anchor to avoid some duplication and removed the start_year and end_year keys from the fx variable, because it probably doesn't have a start and end year.

[koldunovn]

@bouweandela Thanks a lot for the review! I will test the changes in the next couple of days.

[koldunovn]

Hi @bouweandela

I tested the diagnostic with all suggested changes and the latest master and it works for me at DKRZ. Do you think you can give it a go, so @mattiarighi test it?

[koldunovn]

The change in the YAML file does not work, it gives:

Missing keys {'start_year', 'end_year'} from variable areacello in diagnostic arctic_ocean

I would keep it as is. Other diagnostics also use "fake" time range for areacello.

[mattiarighi]

The change in the YAML file does not work

Did you update your ESMValCore installation to the latest version?

[koldunovn]

The change in the YAML file does not work

Did you update your ESMValCore installation to the latest version?

I merged with master and did:

conda env update --name esmvaltool --file environment.yml
pip install .

That should give me the latest released version (available from pip), I guess :)

[koldunovn]

This is what I get now:

esmvalcore.__version__
'2.0.0b3'

[valeriupredoi]

not from pip remote but from local - did you git pull the latest master? Note that conda env update is not needed unless dependencies have changed

[koldunovn]

not from pip remote but from local - did you git pull the latest master? Note that conda env update is not needed unless dependencies have changed

By master I mean ESMValTool master, sorry. I use ESMValCore from the pip.

[mattiarighi]

That is probably the problem.
Try to install the core in development mode, see instructions.

That would give you the very last version.

[koldunovn]

That is probably the problem.
Try to install the core in development mode, see instructions.

That would give you the very last version.

But isn't it a bit inconsistent? I thought the idea of having Core and Tools separated is to make it possible to develop diagnostics against something stable, not a moving target? If this diagnostic is changed to work with the latest version, merged and the user will try to use it the next day, it will not work (in this particular case at least), because he/she will get 2.0.0b3 by following instructions in the documentation.

On the other hand, one would like to have all diagnostics working with the latest version of the tool, that's understandable. I guess the way the YAML file is now will work in both cases (2.0.0b3 and the latest).

Let me know how to proceed - I can, of course, install the bleeding edge version of ESMValCore and test against it. Or maybe it is time to bump the version of ESMValCore?

[valeriupredoi]

yes, but when we'll have a stable major release then we'll use that one. Also @bouweandela could update the tagged minor releases for Core more often :grin: :imp:

[mattiarighi]

But isn't it a bit inconsistent? I thought the idea of having Core and Tools separated is to make it possible to develop diagnostics against something stable, not a moving target?

But in this case you are (probably) trying to use a feature which has not been released yet.

Let me know how to proceed - I can, of course, install the bleeding edge version of ESMValCore and test against it.

Yes, let's try to see if it solves the problem.

[koldunovn]

But in this case you are (probably) trying to use a feature which has not been released yet.

It's not like I want to :) I am fine with the current version.

Yes, let's try to see if it solves the problem.

Sure, I will try it now.

[mattiarighi]

It's not like I want to :) I am fine with the current version.

What I mean is that the issue with the missing keys in areacello is related to something that was recently changed in ESMValCore and I can't remember whether it is already in the latest release. ;-)

[koldunovn]

Well, after installing the latest version of ESMValCore the suggestion from @bouweandela works, and of course it more readable and understandable, when setup is formulated this way.

But again, this version not going to work with current stable :) Let me know if I should commit this version of YAML file, or stick to the previous one :stuck_out_tongue_winking_eye:

[zklaus]

Well, the current stable is ESMValTool-1.1.0 since after that there were only pre-releases. Though I admit that the current beta release is impressively stable already.

[mattiarighi]

Let me know if I should commit this version of YAML file, or stick to the previous one

Just go ahead if it works.
There will be anyway a new release pretty soon.

[valeriupredoi]

we don't talk about 1.1.0 :hushed:

[koldunovn]

Well, the current stable is ESMValTool-1.1.0 since after that there were only pre-releases. Though I admit that the current beta release is impressively stable already.

Of course, by "stable", I mean tagged release, or more generally the latest ESMValCore you get from pypi. You right there is no stable version 2 yet :wink:

[koldunovn]

I added the new version of the yaml file. The code is working and all tests are green (except couple of acceptable codacy issues). @bouweandela @mattiarighi please let me know what actions are further necessary from my side.

[bouweandela]

Thanks @koldunovn! Looks good from my side. I will make a new core release soon, I have to find some time to collect the release notes. There is no ESMValCore or ESMValTool release on PyPI yet, the packages are only available on our own conda channel. I will make a PyPI package as soon as I get the green light to make a real version 2 release (instead of beta).