Extensions to PR #763

[ledm]

Comments from @bouweandela about PR ESMValGroup/ESMValTool#763.

There are still several things to be done, but if you're really a in hurry to merge, @ledm can put them in an issue and do it later:

Preprocessor 1) I realised this only late, but the new preprocessor functionality is not covered by unit tests, this reduces the reliability and maintainability of the preprocessor and should be addressed. 2) There new fx_files argument for those two preprocessor functions is still strange: why do you have a dictionary in which you will only ever expect one item. I think it should just be fx_file and the argument should be the name of the file or None if you don't have a file. 3) There are Codacy issues remaining, the complexity/too many local variables issues are serious because they make the volume_average function difficult to understand and maintain

esmvaltool/preprocessor/_area_pp.py
Use % formatting in logging functions and pass the % parameters as arguments
179
logger.info('Calculated grid area:{}'.format(grid_areas.shape))

esmvaltool/preprocessor/_volume_pp.py
Too many local variables (24/15)
162

def volume_average(

volume_average is too complex (11)
162

def volume_average(

Ocean Diagnostics package

A lot of duplication i.e. copy/paste style of coding is still reported by Codacy, https://github.com/ESMValGroup/ESMValTool/pull/763#issuecomment-457579983. This code should be moved into functions to keep it maintainable. If you later want to change something about this code, you will have to search all over the place for copies.

[schlunma]

Addition to point 2.:

If you want to use a fx file for area averaging right now, areacello is hardcoded:

https://github.com/ESMValGroup/ESMValTool/blob/6c72cc1a4f71b651d31333876715dfb23d67bdb0/esmvaltool/_recipe.py#L402-L413

This fails for non-ocean variables, which is particularly bad since this is the default behaviour if fx_file is not given. Right now, the ECS diagnostic of recipe_flato13ipcc.yml fails because of this.

[ledm]

HI @schlunma, good spot!

Would it work if you add areacella to the list? ie:

             var['fx_files'] = ['areacello', `areacella` ] 

[schlunma]

It could work for certain cases, but not in general.

https://github.com/ESMValGroup/ESMValTool/blob/6c72cc1a4f71b651d31333876715dfb23d67bdb0/esmvaltool/preprocessor/_area_pp.py#L149-L169

Here, grid_areas is set to the last fx_file in the dictionary, so this depends on the order of appearance in 'fx_files'.

My suggestion is to use iris.analysis.cartography.area_weights if the option fx_files is None or not given and load a user-defined fx variable if specified.

[ledm]

Hi @schlunma,

I've created a basic branch which should address this:

https://github.com/ESMValGroup/ESMValTool/tree/version2_development_ocean_extension_817

The main thing that it does is to delete the following lines from _recipe.py:

         if step == 'average_region': 
             var['fx_files'] = ['areacello', ] 
         if step == 'average_volume': 
             var['fx_files'] = ['volcello', ]

These lines overwrite the fx_files specified in the recipe. This is bad. I'm sorry that it made it into the main dev branch.

Note that both the volume_average and area_average preprocessors do as you suggest. They called iris.analysis.cartography or similar if the option fx_files is not given.

I think that it might be good to patch this in as a really quick PR.

[valeriupredoi]

the issues related to this issue seem to have been addressed - eh @ledm ?

[bouweandela]

Except for this bit:

Ocean Diagnostics package A lot of duplication i.e. copy/paste style of coding is still reported by Codacy, #763 (comment). This code should be moved into functions to keep it maintainable. If you later want to change something about this code, you will have to search all over the place for copies.

[bouweandela]

1 I realised this only late, but the new preprocessor functionality is not covered by unit ests, this reduces the reliability and maintainability of the preprocessor and should be addressed.

I've just had a look, but unit tests are still half missing for the preprocessor functions introduced here. @ledm Would you have time to look at this? You can view the coverage locally by checking out the ESMValTool Core repository and running:

python setup.py test
firefox test-reports/coverage_html/index.html

We should soon have the coverage results available on Codacy again. In the mean time, they are available on CircleCI if you click the artifacts tab, see here: https://circleci.com/gh/ESMValGroup/ESMValCore/43#artifacts/containers/0 https://43-190192145-gh.circle-artifacts.com/0/install/test-reports/coverage_html/index.html