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

a diagnostic to evaluate the turnover times of land ecosystem carbon #1395

Merged
merged 67 commits into from Jul 28, 2020

Conversation

dr-ko
Copy link
Contributor

@dr-ko dr-ko commented Oct 17, 2019

…stock

Before you start, read CONTRIBUTING.md and the guide for diagnostic developers.

Please discuss your idea with the development team before getting started, to avoid disappointment later. The way to do this is to open a new issue on GitHub. If you are planning to modify existing functionality, please discuss it with the original author(s) by tagging them in the issue.


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

New recipe/diagnostic

  • Add documentation for the recipe to the doc/sphinx/source/recipes folder and add a new entry to index.rst

If you need help with any of the tasks above, please do not hesitate to ask by commenting in the issue or pull request.


Closes #1393

@mattiarighi mattiarighi changed the base branch from version2_development to master Jan 3, 2020
Copy link
Contributor

@zklaus zklaus left a comment

I put some comments with regards to the documentation. Will take a look at the code now.

doc/sphinx/source/recipes/recipe_carvalhais2014nat.rst Outdated Show resolved Hide resolved
doc/sphinx/source/recipes/recipe_carvalhais2014nat.rst Outdated Show resolved Hide resolved
doc/sphinx/source/recipes/recipe_carvalhais2014nat.rst Outdated Show resolved Hide resolved
doc/sphinx/source/recipes/recipe_carvalhais2014nat.rst Outdated Show resolved Hide resolved
doc/sphinx/source/recipes/recipe_carvalhais2014nat.rst Outdated Show resolved Hide resolved
doc/sphinx/source/recipes/recipe_carvalhais2014nat.rst Outdated Show resolved Hide resolved
doc/sphinx/source/recipes/recipe_carvalhais2014nat.rst Outdated Show resolved Hide resolved
doc/sphinx/source/recipes/recipe_carvalhais2014nat.rst Outdated Show resolved Hide resolved
doc/sphinx/source/recipes/recipe_carvalhais2014nat.rst Outdated Show resolved Hide resolved
doc/sphinx/source/recipes/recipe_carvalhais2014nat.rst Outdated Show resolved Hide resolved
@zklaus zklaus force-pushed the CRESCENDO_land_carbon_cycle branch from 6b9f64b to 509679c Compare Mar 24, 2020
Sujan Koirala and others added 25 commits Apr 3, 2020
Depending on the python version, "/" will give a float,
even with two int args. This version uses "//" + casting
to avoid that problem and reduces some redundancy.
- wrapped lines to less than 80 chars
- added newline at end of file
- removed trailing whitespace
@mattiarighi
Copy link
Contributor

mattiarighi commented Jul 23, 2020

In this case the cmorizer script should be provided, so that everyone is able to generate the observational data for the recipe after retrieving the original data from the respective source.

See here.

@mattiarighi
Copy link
Contributor

mattiarighi commented Jul 24, 2020

This PR should also be in the final release, since it's part of one of the already accepted ESMValTool papers.

There are however two issues to be solved:

  • the cmorizer for the observation is apparently missing
  • the recipe name does not follow the standard filenaming convention of the tool (easy fix)

@koir-su any chance you can solve these issues before the final release (end of July)?

@dr-ko
Copy link
Contributor Author

dr-ko commented Jul 24, 2020

Hi Mattia, we have had several discussions on the observation. The cmorization of observation is not a straightforward one here, as the variable compared is not a standard observation but rather a derived variable that is very specific. On top of that, every figure needs a derivation of the observation at the spatial and regional scale. For example, the uncertainty ranges cannot be aggregated, as has been discussed elsewhere within ESMValTool. So, the scripts to prepare observation is not a cmorization script but a rather complicated collection of derivations that produces the data up to the standard needed in the ESMValTool. Moreover, the derived observation-based data are provided in 11 different resolutions so that the evaluators have enough flexibility.

I have pushed the changes to the filenames, as you suggested.

@bouweandela
Copy link
Member

bouweandela commented Jul 24, 2020

Regarding previous discussion on the observations, it took place here #1395 (comment) and here #1395 (comment)

@mattiarighi
Copy link
Contributor

mattiarighi commented Jul 24, 2020

matplotlib is complaining...

Traceback (most recent call last):
  File "/mnt/lustre01/pf/b/b309057/ESMValTool/public/esmvaltool/diag_scripts/land_carbon_cycle/diag_global_turnover.py", line 824, in <module>
    main(config)
  File "/mnt/lustre01/pf/b/b309057/ESMValTool/public/esmvaltool/diag_scripts/land_carbon_cycle/diag_global_turnover.py", line 788, in main
    _plot_multimodel_agreement(plot_path_multimodel, global_tau_mod,
  File "/mnt/lustre01/pf/b/b309057/ESMValTool/public/esmvaltool/diag_scripts/land_carbon_cycle/diag_global_turnover.py", line 614, in _plot_multimodel_agreem
ent
    col_bar = plut.mk_colo_cont(_axcol_rat,
  File "/mnt/lustre01/pf/b/b309057/ESMValTool/public/esmvaltool/diag_scripts/land_carbon_cycle/plot_utils.py", line 169, in mk_colo_cont
    norm=mpl.colors.BoundaryNorm(
  File "/work/bd0080/b309057/SOFTWARE/miniconda3/envs/esmvaltool/lib/python3.8/site-packages/matplotlib/colors.py", line 1486, in __init__
    raise ValueError(f"There are {self._N} color bins including "
ValueError: There are 287 color bins including extensions, but ncolors = 256; ncolors must equal or exceed the number of bins

@bouweandela
Copy link
Member

bouweandela commented Jul 24, 2020

the scripts to prepare observation is not a cmorization script but a rather complicated collection of derivations that produces the data

Are these scripts available somewhere publicly? In the interest of transparency and reproducibility, it would be really good if these could be made available somewhere, along with instructions on how to obtain the original input data and how to use the scripts.

@dr-ko
Copy link
Contributor Author

dr-ko commented Jul 24, 2020

Damn! I use 2.2.4, and it does not complain. Can you please send me the yml for your esmvaltool environment so that I can try to fix it within that...

@dr-ko
Copy link
Contributor Author

dr-ko commented Jul 24, 2020

Are these scripts available somewhere publicly? In the interest of transparency and reproducibility, it would be really good if these could be made available somewhere, along with instructions on how to obtain the original input data and how to use the scripts.

No, but I can add the programs as is to a git repo. The full data will not be available publicly for the reasons mentioned before. The uncertainty bounds are based on the full cube of all realizations from Carvalhais, which is not available for public use (outside my hands).
For the zonal means and correlations, the processing script is exactly the same as implemented in ESMValTool for models.

@mattiarighi
Copy link
Contributor

mattiarighi commented Jul 24, 2020

Can you please send me the yml for your esmvaltool environment so that I can try to fix it within that...

https://github.com/ESMValGroup/ESMValTool/blob/master/environment.yml

and I'm using

matplotlib-base           3.3.0            py38hb801037_0    conda-forge

@dr-ko
Copy link
Contributor Author

dr-ko commented Jul 27, 2020

@mattiarighi, I have fixed the issue with the matplotlib version. Can you give it another go?

@mattiarighi mattiarighi mentioned this pull request Jul 27, 2020
158 tasks
@mattiarighi
Copy link
Contributor

mattiarighi commented Jul 27, 2020

There is something wrong with the provenance:

"""
Traceback (most recent call last):
  File "/work/bd0080/b309057/SOFTWARE/miniconda3/envs/esmvaltool/lib/python3.8/multiprocessing/pool.py", line 125, in worker
    result = (True, func(*args, **kwds))
  File "/work/bd0080/b309057/SOFTWARE/miniconda3/envs/esmvaltool/lib/python3.8/site-packages/esmvalcore/_task.py", line 730, in _run_task
    output_files = task.run()
  File "/work/bd0080/b309057/SOFTWARE/miniconda3/envs/esmvaltool/lib/python3.8/site-packages/esmvalcore/_task.py", line 242, in run
    self.output_files = self._run(input_files)
  File "/work/bd0080/b309057/SOFTWARE/miniconda3/envs/esmvaltool/lib/python3.8/site-packages/esmvalcore/_task.py", line 520, in _run
    self._collect_provenance()
  File "/work/bd0080/b309057/SOFTWARE/miniconda3/envs/esmvaltool/lib/python3.8/site-packages/esmvalcore/_task.py", line 590, in _collect_provenance
    if ancestor_file in ancestor_products:
TypeError: unhashable type: 'list'
"""

@bouweandela
Copy link
Member

bouweandela commented Jul 27, 2020

@mattiarighi Can you attach the log so we can see which diagnostic it is? This happens when ancestors is a list of list of strings instead of a list of strings.

@mattiarighi
Copy link
Contributor

mattiarighi commented Jul 27, 2020

@bouweandela
Copy link
Member

bouweandela commented Jul 27, 2020

Could you also attach /mnt/lustre02/work/bd0854/b309057/ESMValTool/v2_output/recipe_carvalhais14nat_20200727_153204/run/diag_land_carbon_turnover/global_turnover_time/diagnostic_provenance.yml? It looks like the problem is in diag_land_carbon_turnover/global_turnover_time.

@mattiarighi
Copy link
Contributor

mattiarighi commented Jul 27, 2020

@bouweandela
Copy link
Member

bouweandela commented Jul 27, 2020

Ok, so the problem is in the files related to the figure with caption "Map of global distribution of turnover time of carbon". @koir-su Can you have look?

@dr-ko
Copy link
Contributor Author

dr-ko commented Jul 27, 2020

Thanks, both. Apparently, the problem was due to having a list as ancestor_files. Needed dictionary. Strangely, was not a problem with the older version of ESMValTool.

@mattiarighi
Copy link
Contributor

mattiarighi commented Jul 28, 2020

Thank you, it works now.

@bouweandela please cherry-pick.

@mattiarighi mattiarighi merged commit 0665ca6 into master Jul 28, 2020
1 check failed
@mattiarighi mattiarighi deleted the CRESCENDO_land_carbon_cycle branch Jul 28, 2020
@dr-ko
Copy link
Contributor Author

dr-ko commented Jul 28, 2020

Thank you all for such a thorough review. I can say that I learned a lot from this. 🍻

@bouweandela
Copy link
Member

bouweandela commented Jul 29, 2020

please cherry-pick.

@mattiarighi Done

No, but I can add the programs as is to a git repo. The full data will not be available publicly for the reasons mentioned before. The uncertainty bounds are based on the full cube of all realizations from Carvalhais, which is not available for public use (outside my hands).

@koir-su It would be great if you could do this and add a link in the recipe documentation. Make sure to add Zenodo integration to the repository and then make a release, so they have a DOI. If you have no intention of changing the scripts, you could also directly upload them to Zenodo.

bouweandela added a commit that referenced this pull request Jul 29, 2020
…1395)

* a diagnostic to evaluate the turnover times of land ecosystem carbon stock

* minor updates of land carbon cycle (recipe_carvalhais2014)

* minor edits in land_carbon_cycle(recipe_carvalhais2014)

* fully running recipe_carvalhais2014 for CRESCENDO_land_carbon_cycle

* changes in handling NorESM resolution data

* Fix float division in plot tick placement

Depending on the python version, "/" will give a float,
even with two int args. This version uses "//" + casting
to avoid that problem and reduces some redundancy.

* fixing issues with full run

* minor fixes in all diagnostic scripts

* additional documentation and full run

* test compatibility

* Move shared code to shared module

* Remove dot dict and reformat issues

* Simplify recipe

* Simplify configuration handling

* Refactor zonal turnover calculation to use cubes

* Improved documentation

* Improved formatting of documentation (see below)

- wrapped lines to less than 80 chars
- added newline at end of file
- removed trailing whitespace

* Fix docstring bug

* Unified zonal observation handling

* handling obs res

* update of recipe documentation, clean up of obsolete fig_config, and minor formatting of code

* Fix recipe

* Fix zonal turnover calculation

* Minor formatting issues

* Minor formatting issues

* Remove left-overs

* Minor fixes

* Minor formatting

* Formatted documentation

* Fixed reference link

* Fixed math formatting

* Fix Obs4MIPs link in documentation

* Removed `.. _user settings:` anchor

* Corrected tag in recipe

* Updated figures of documentation

* update on documentation and figures

* update on documentation and figures

* minor formatting for CI tests

* Cleanup

* Add provenance information for diag_zonal_turnover

* Cleanup

* provenance information for diag_zonal_correlation

* provenance of diag_global_turnover

* cleanup

* minor changes in figure titles and labels

* incorporate comments from pull request review and update of figures

* cleanup for circleci test

* circleci test fix

* additional codacy checks fix and provenance update

* update documentation regarding preprocessed observation at different resolutions

* bug fix in location of doc file

* fix provenance handling

* fix codacy issues

* fix generic variable names

* fixing and cleaning provenance logging

* Fix provenance if no netcdf output is requested

* renaming recipe filenames to the standard

* Fix recipe name in index

* Add a note on data availability

* fixing matplotlib issue with global

* fix provenance issue with ancestor files

Co-authored-by: Sujan Koirala <b301040@mlogin102.hpc.dkrz.de>
Co-authored-by: Sujan Koirala <b301040@mlogin108.hpc.dkrz.de>
Co-authored-by: Sujan Koirala <b301040@mistralpp2.hpc.dkrz.de>
Co-authored-by: Klaus Zimmermann <klaus.zimmermann@smhi.se>
Co-authored-by: Sujan Koirala <b301040@mlogin100.hpc.dkrz.de>
Co-authored-by: Sujan Koirala <b301040@mlogin105.hpc.dkrz.de>
Co-authored-by: koir-su <b301040@mlogin103.hpc.dkrz.de>
Co-authored-by: koir-su <b301040@mlogin104.hpc.dkrz.de>
Co-authored-by: koir-su <b301040@mlogin101.hpc.dkrz.de>
Co-authored-by: Bouwe Andela <b.andela@esciencecenter.nl>
Co-authored-by: Mattia Righi <mattia.righi@dlr.de>
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.

a diagnostic for evaluation of ecosystem carbon turnover times
5 participants