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

Solved issues in configuration files #1457

Merged
merged 8 commits into from Feb 3, 2022
Merged

Solved issues in configuration files #1457

merged 8 commits into from Feb 3, 2022

Conversation

schlunma
Copy link
Contributor

@schlunma schlunma commented Feb 2, 2022

Description

This fixes the issues raise in #1456 and also synchronizes the config-user.yml file with the corresponding section in the documentation.

Closes #1456

Link to documentation: https://esmvaltool--1457.org.readthedocs.build/projects/ESMValCore/en/1457/quickstart/configure.html#user-configuration-file


Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the πŸ›  Technical or πŸ§ͺ Scientific review.


To help with the number pull requests:

@schlunma schlunma added the documentation Improvements or additions to documentation label Feb 2, 2022
@schlunma schlunma added this to the v2.5.0 milestone Feb 2, 2022
@schlunma schlunma self-assigned this Feb 2, 2022
@schlunma
Copy link
Contributor Author

schlunma commented Feb 2, 2022

@bouweandela Please add your comments to #1453 here.

I also noticed that some of the options given in esmvalcore/_config/_config.py are not given in config-user.yml (extra_facets_dir, resume_from, and run_diagnostic). This is kind of related to #1433, and I'm not sure if we should list them or not.

@codecov
Copy link

codecov bot commented Feb 2, 2022

Codecov Report

Merging #1457 (80cc986) into main (7d43072) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1457      +/-   ##
==========================================
- Coverage   89.67%   89.66%   -0.01%     
==========================================
  Files         196      196              
  Lines       10339    10339              
==========================================
- Hits         9271     9270       -1     
- Misses       1068     1069       +1     
Impacted Files Coverage Ξ”
esmvalcore/_config/_config.py 95.48% <ΓΈ> (-0.76%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data
Powered by Codecov. Last update 7d43072...80cc986. Read the comment docs.

@bouweandela
Copy link
Member

I also noticed that some of the options given in esmvalcore/_config/_config.py are not given in config-user.yml (extra_facets_dir, resume_from, and run_diagnostic). This is kind of related to #1433, and I'm not sure if we should list them or not.

I think it's fine to keep it as is. For extra_facets_dir we thought this option was too advanced for the example file, resume_from cannot have a meaningful default value as far as I know. Maybe run_diagnostic could?

In the longer term, the plan is to replace esmvalcore/_config/_config.py with esmvalcore/experimental/config which uses the defaults from config-user.yml instead of maintaining it's own copy:

mapping = _read_config_file(filename)
# Add defaults that are not available in esmvalcore/config-user.yml
mapping['extra_facets_dir'] = tuple()
mapping['resume_from'] = []

Copy link
Member

@bouweandela bouweandela left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes @schlunma, the documentation look much more neat now.

doc/quickstart/configure.rst Outdated Show resolved Hide resolved
doc/quickstart/configure.rst Outdated Show resolved Hide resolved
doc/quickstart/configure.rst Outdated Show resolved Hide resolved
schlunma and others added 4 commits February 3, 2022 11:43
Co-authored-by: Bouwe Andela <b.andela@esciencecenter.nl>
Co-authored-by: Bouwe Andela <b.andela@esciencecenter.nl>
Co-authored-by: Bouwe Andela <b.andela@esciencecenter.nl>
Copy link
Member

@bouweandela bouweandela left a comment

Choose a reason for hiding this comment

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

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configuration file issues
2 participants