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 various fixes for hybrid pressure coordinates #529

Merged
merged 4 commits into from
Mar 12, 2020

Conversation

schlunma
Copy link
Contributor

@schlunma schlunma commented Mar 2, 2020

Added various fixes for CMIP5 and CMIP6 models concerning hybrid pressure coordinates (variables cl, cli and clw).

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)
  • This pull request has a descriptive title that can be used in a changelog
  • Add unit tests
  • Public functions should have a numpy-style docstring so they appear properly in the API documentation. For all other functions a one line docstring is sufficient.
  • If writing a new/modified preprocessor function, please update the documentation
  • Circle/CI tests pass. Status can be seen below your pull request. If the tests are failing, click the link to find out why.
  • Codacy code quality checks pass. 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
  • If you make backward incompatible changes to the recipe format, make a new pull request in the ESMValTool repository and add the link below

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


Related to #536, #540, #541, #542. (Note: Automatic closing of these issues is not desired; they should be closed when the issue is reported to the modelers).

@schlunma

This comment has been minimized.

@schlunma

This comment has been minimized.

@schlunma schlunma changed the title Added various fixes for CMIP5 and CMIP6 models Added various fixes for hybrid pressure coordinates Mar 9, 2020
@schlunma

This comment has been minimized.

Copy link

@zklaus zklaus left a comment

Choose a reason for hiding this comment

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

Left some comments, did not review the tests yet.

esmvalcore/cmor/_fixes/cmip5/bcc_csm1_1.py Outdated Show resolved Hide resolved
esmvalcore/cmor/_fixes/cmip5/bcc_csm1_1.py Outdated Show resolved Hide resolved
esmvalcore/cmor/_fixes/cmip5/bcc_csm1_1.py Outdated Show resolved Hide resolved
esmvalcore/cmor/_fixes/cmip5/bcc_csm1_1_m.py Outdated Show resolved Hide resolved
esmvalcore/cmor/_fixes/cmip6/bcc_csm2_mr.py Outdated Show resolved Hide resolved
esmvalcore/preprocessor/_derive/__init__.py Show resolved Hide resolved
esmvalcore/cmor/_fixes/cmip5/bcc_csm1_1.py Outdated Show resolved Hide resolved
esmvalcore/cmor/_fixes/cmip5/bcc_csm1_1.py Outdated Show resolved Hide resolved
@schlunma
Copy link
Contributor Author

schlunma commented Mar 12, 2020

@zklaus I rebased the master and added your suggestions.

Please not that I intentionally did not use

from ..common import ClFixHybridPressureCoord as Cl

but

from ..common import ClFixHybridPressureCoord

Cl = ClFixHybridPressureCoord

for two reasons: First, to mute Codacy and second, to make it clearer that there is a fix for Cl in the file, otherwise I think that it might not be so obvious.

Copy link

@zklaus zklaus left a comment

Choose a reason for hiding this comment

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

I didn't check all tests in detail, but to me it seems fine.

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.

Same here.

@mattiarighi mattiarighi merged commit 805e5f4 into master Mar 12, 2020
@mattiarighi mattiarighi deleted the add_various_fixes branch March 12, 2020 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix for dataset Related to dataset-specific fix files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants