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

Added additional emergent constraints on ECS #1585

Merged
merged 17 commits into from Jul 27, 2020
Merged

Conversation

schlunma
Copy link
Contributor

@schlunma schlunma commented Mar 16, 2020

Add several emergent constraints (including lots of code for general emergent constraints) for ECS, among them the Volodin (2008) constraint necessary for Lauer et al.

Depends on ESMValGroup/ESMValCore#546 and ESMValGroup/ESMValCore#562.


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
  • (Only if really necessary) Add any additional dependencies needed for the diagnostic script to setup.py, esmvaltool/install/R/r_requirements.txt or esmvaltool/install/Julia/julia_requirements.txt (depending on the language of your script) and also to meta.yaml for conda dependencies (includes Python and others, but not R/Julia)
  • If new dependencies are introduced, check that the license is compatible with Apache2.0

New recipe/diagnostic

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

Closes #1584.

@axel-lauer

This comment has been minimized.

@schlunma

This comment has been minimized.

@schlunma

This comment has been minimized.

@schlunma schlunma marked this pull request as ready for review June 23, 2020 14:18
@schlunma

This comment has been minimized.

Copy link
Contributor

@axel-lauer axel-lauer left a comment

Choose a reason for hiding this comment

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

I tested this branch and it works. The figures produced look fine. What I find confusing is the need fo a file called "ecs_emergent_constraints.yml in your auxiliary_data_dir". Following the instructions given in this pull request, I created such a file and put it into the auxiliary_data_dir specified in config-user.yml. The recipe then crashed as the file could not be found. When copying it to the ESMValTool base directory, the recipe worked. I then tried to remove the key "read_external_file" from the recipe as the documentation says this would be optional. Again, the recipe crashed.

I would therefore strongly suggest to incorporate the content of "ecs_emergent_constraints.yml" into the main recipe (if possible) or introduce default settings that make providing such a file indeed optional.
If this is not possible, the search path for ecs_emergent_constraints.yml would have to be double-checked and a description of how to create the content of this file would have to be added to the documentation.

provenance_authors: ['schlund_manuel']
provenance_domains: ['global']
provenance_realms: ['atmos']
provenance_references: ['gregory04grl']
provenance_statistics: ['mean', 'anomaly']
provenance_themes: ['phys']
Copy link
Member

Choose a reason for hiding this comment

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

themes and realms is usually included directly under the diagnostic in the recipe, have a look at how it's done in e.g. recipe_perfmetrics_CMIP5.yml or examples/recipe_python.yml. The other attributes are usually set in the code, because they are very closely tied to the diagnostic script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the diagnostic univariate_constraint.py it does not make sense to hardcode the provenance information in the header of the recipe or the diagnostic script itself, since it is able to handle arbitrary emergent constraints. E.g., realms could be anything (atmosphere, ocean, land, etc.).

Copy link
Member

Choose a reason for hiding this comment

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

But you will still be the author of the diagnostic script, right?

@bouweandela

This comment has been minimized.

@schlunma schlunma added the requires new ESMValCore release A new release of ESMValCore is needed to solve this issue/merge this pull request.. label Jul 14, 2020
@schlunma
Copy link
Contributor Author

@axel-lauer I have added the contents of the external file to the recipe, so the external file is not necessary anymore. I also addressed the other comments by @bouweandela. Please note that this PR needs a new release of ESMValCore to work.

Copy link
Contributor

@axel-lauer axel-lauer left a comment

Choose a reason for hiding this comment

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

Tested the latest version of recipe_ecs_constraints.yml and recipe_ecs_scatter.yml (as this could have been affected by changes to ecs_scatter.ncl). Looks all good! Thanks for moving the config options from the external file to the recipe.

@mattiarighi
Copy link
Contributor

I get an error in recipe_ecs_constraints.yml

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 523, in _run
    raise DiagnosticError(
esmvalcore._task.DiagnosticError: Diagnostic script emergent_constraints/ecs_scatter.py failed with return code 1. See the log in /mnt/lustre02/work/bd0854/b309057/ESMValTool/v2_output/recipe_ecs_constraints_20200721_092906/run/diag_x_volodin_cmip5/ecs_predictor/log.txt

In the log:

Traceback (most recent call last):
  File "/mnt/lustre01/pf/b/b309057/ESMValTool/public/esmvaltool/diag_scripts/emergent_constraints/ecs_scatter.py", line 59, in <module>
    from esmvalcore.cmor import add_plev_from_altitude, add_sigma_factory
ImportError: cannot import name 'add_plev_from_altitude' from 'esmvalcore.cmor' (/work/bd0080/b309057/SOFTWARE/miniconda3/envs/esmvaltool/lib/python3.8/site-
packages/esmvalcore/cmor/__init__.py)

@bouweandela
Copy link
Member

Yes, see #1585 (comment)

@mattiarighi
Copy link
Contributor

@schlunma can you fix this please?

@bouweandela bouweandela removed the requires new ESMValCore release A new release of ESMValCore is needed to solve this issue/merge this pull request.. label Jul 22, 2020
@mattiarighi
Copy link
Contributor

mattiarighi commented Jul 23, 2020

I tested successfully both recipes.
@bouweandela if you are ok with this please merge and cherry pick to v2.0.0-release.

@bouweandela
Copy link
Member

The way the provenance and tags are implemented in these diagnostics is different from how it's normally done and I'm not convinced it's the right way to go, see also #1585 (comment). @mattiarighi Do you want to fix this now, or shall we make an issue so @schlunma can fix it when he's back from holiday?

@mattiarighi
Copy link
Contributor

The problem is that this needs to go in the final release because it's part of @axel-lauer's paper.

What about merging it now and opening an issue that @schlunma can take care of when he is back?

@mattiarighi mattiarighi merged commit 414ef7a into master Jul 27, 2020
1 check failed
@mattiarighi mattiarighi added this to Done in High priority issues via automation Jul 27, 2020
@mattiarighi mattiarighi deleted the add_ecs_constraints branch July 27, 2020 15:22
@mattiarighi
Copy link
Contributor

@bouweandela please cherry-pick in v2.0.0-release.

bouweandela added a commit that referenced this pull request Jul 27, 2020
* Added additional emergent constraints on ECS

* Added missing reference

* Simplified LTMI calculation using S and D functinos

* Fixed codacy issue

* Fixed parallel calculation of pressure level widths

* Added additional emergent constraints on ECS

* Added missing reference

* Simplified LTMI calculation using S and D functinos

* Fixed codacy issue

* Fixed parallel calculation of pressure level widths

* Moved additional data from external file to recipe

* Add #ESMValTool to header

* Fix import path

* Fix imports

Co-authored-by: Mattia Righi <mattia.righi@dlr.de>
Co-authored-by: Bouwe Andela <b.andela@esciencecenter.nl>
@bouweandela
Copy link
Member

Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Add emergent constraint for ECS of Volodin (2008)
4 participants