Skip to content

Added various fixes for hybrid pressure coordinates #529

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.

Sign up for GitHub

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

Jump to bottom
Merged
mattiarighi merged 4 commits into from
Mar 12, 2020
Merged

Added various fixes for hybrid pressure coordinates #529

mattiarighi merged 4 commits into from
Mar 12, 2020

Conversation

@schlunma
Copy link
Contributor

@schlunma schlunma commented Mar 2, 2020 •
edited
Loading

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 schlunma added the fix for dataset Related to dataset-specific fix files label Mar 2, 2020
@schlunma schlunma self-assigned this Mar 2, 2020
@schlunma schlunma force-pushed the add_various_fixes branch from cc2e09c to 9c0abdf Compare March 3, 2020 13:40
This was referenced Mar 3, 2020
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@schlunma schlunma marked this pull request as ready for review March 3, 2020 15:31
@schlunma

This comment has been minimized.

Sign in to view
@schlunma

This comment has been minimized.

Sign in to view
@schlunma schlunma force-pushed the add_various_fixes branch from f0643c1 to 0dfb43c Compare March 9, 2020 12:32
@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.

Sign in to view
@schlunma schlunma mentioned this pull request Mar 9, 2020
Closed
@schlunma schlunma added this to the ESMValTool papers milestone Mar 10, 2020
zklaus
zklaus reviewed Mar 11, 2020
View reviewed changes
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.

@schlunma schlunma mentioned this pull request Mar 12, 2020
Merged
9 tasks
zklaus
zklaus reviewed Mar 12, 2020
View reviewed changes
@schlunma schlunma force-pushed the add_various_fixes branch from 0dfb43c to 8b10380 Compare March 12, 2020 11:53
@schlunma
Copy link
Contributor Author

schlunma commented Mar 12, 2020 •
edited
Loading

@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.

zklaus
zklaus approved these changes Mar 12, 2020
View reviewed changes
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.

bouweandela
bouweandela approved these changes Mar 12, 2020
View reviewed changes
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

Reviewers

@zklaus zklaus zklaus approved these changes

@bouweandela bouweandela bouweandela approved these changes

@valeriupredoi valeriupredoi Awaiting requested review from valeriupredoi

Assignees

@schlunma schlunma

Labels

fix for dataset Related to dataset-specific fix files

Projects

None yet

Milestone

ESMValTool papers

Development

Successfully merging this pull request may close these issues.

5 participants

@schlunma @zklaus @bouweandela @mattiarighi